RuleContext: use correct override.

[hzeller]

The operator== in RuleContext only mildly looked like the virtual operator== in the base class: it does take a different parameter and was not const.

This results in warnings by the compiler complaining about a shadowed method:

/usr/include/antlr4-runtime/tree/ParseTree.h:50:18: warning: ‘virtual bool antlr4::tree::ParseTree::operator==(const antlr4::tree::ParseTree&) const’ was hidden [-Woverloaded-virtual=]
   50 |     virtual bool operator == (const ParseTree &other) const;
      |                  ^~~~~~~~
/usr/include/antlr4-runtime/RuleContext.h:135:10: note:   by ‘bool antlr4::RuleContext::operator==(const antlr4::RuleContext&)’
  135 |     bool operator == (const RuleContext &other) { return this == &other; } // Simple address comparison.

This does not look intentional. Fix by using the correct parameter (which is the base class) and mark it const and override.

[hzeller]

Adding @mike-lischke as the original author of this class ca. 2016. Does this look like the correct conclusion and fix ?

[mike-lischke]

Well, it is intentionally written like this and has served well over time. The purpose is to narrow down the allowed parameter type to something that makes more sense. Why should we allow ParseTree here, knowing that it would often not match (e.g. a TerminalNode also is a ParseTree)? If we don't allow anything more basic than RuleContext, we don't need to check for the class type.

However in this special case we are only comparing addresses, so it would not add additional work to check for equality. It still would create the impression any ParseTree instance is valid here, which I'd like to avoid.

[hzeller]

I see what the plausible intention was, but I also wonder if it would've been called given that it is a non-const operator.
So it looks like to a human, but I wonder if the operator would ever be selected ?

If I remove the body and just declare it:

 bool operator == (const RuleContext &other);

... and then compile a largish project with a complicated grammar ... I don't get a linking error for the missing implementation. So it looks like it is never used inside the Antlr implementation; is it expected for users of the API to possibly call ?

[hzeller]

The base clasee, tree::ParseTree implementation of that operator is already simply comparing the pointers, so even if the RuleContext implementation was selected, it does exactly the same thing.

https://github.com/antlr/antlr4/blob/dev/runtime/Cpp/runtime/src/tree/ParseTree.cpp#L10-L12

So my suggestion is now actually to remove the operator in RuleContext to not confuse humans and computers.

[mike-lischke]

You touched an area that can be simplified. The class RuleContext is actually useless. In Java people tend to create complicated class hierarchies and ANTLR is no exception.

In the TypeScript runtime I removed a number of intermediated classes, which were never really needed. One is RuleContext. It's only used as base class for ParserRuleContext, so I made that directly inheriting from ParseTree there. And ParseTree is an interface, so in C++ it could be made a fully abstract class. Consequently all real code lives in ParserRuleContext. And that is not used for object equality checks (which is needed for hash maps and sets). So it neither needs the == operator overload nor the equal() method. And looking at the class shows: it hasn't any of that.

When I come to externalizing target runtimes in my new antlr-ng tool I will probably have to take over the C++ runtime again (unless I find someone to take over as the owner). At that point it's worth to optimize this runtime further and remove such artifacts from the Java conversion.

[mike-lischke]

I guess you don't want to go the extra step and remove RuleContext?

Anyway, for now this change is ok.

[hzeller]

Removing RuleContext sounds like a good next step (and I am down for it as time permits), but it is used in a bunch of places, so cleaning these up should probably be a separate PR.

[mike-lischke]

@parrt This is change can be merged.