RFR: 8340786: Introduce Predicate classes with predicate iterators and visitors for simplified walking [v3]

[Christian Hagedorn]

On Fri, 27 Sep 2024 12:53:13 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add missing public for UnifiedPredicateVisitor
>
> src/hotspot/share/opto/loopnode.cpp line 4325:
> 
>> 4323: class ParsePredicateUsefulMarker : public PredicateVisitor {
>> 4324:  public:
>> 4325:   using PredicateVisitor::visit;
> 
> Why is this needed?

Forgot to explain the reason behind. Without this, compilation fails with:

/home/christian/jdk3/open/src/hotspot/share/opto/predicates.hpp:240:16: error: 'virtual void PredicateVisitor::visit(const InitializedAssertionPredicate&)' was hidden [-Werror=overloaded-virtual=]
  240 |   virtual void visit(const InitializedAssertionPredicate& initialized_assertion_predicate) {}
      |                ^~~~~
/home/christian/jdk3/open/src/hotspot/share/opto/loopnode.cpp:4327:8: note:   by 'virtual void ParsePredicateUsefulMarker::visit(const ParsePredicate&)'
 4327 |   void visit(const ParsePredicate& parse_predicate) override {
      |  

I was not quite sure what this means, so I looked it up and found this:
https://stackoverflow.com/questions/18515183/c-overloaded-virtual-function-warning-by-clang

It looks like the warning is here to prevent accidental hiding of overloads. Here is another example:

struct Base {
  virtual void foo(double e) {
    printf("Base");
  }
};

struct Derived: public Base {
  virtual void foo(int e) {
    printf("Derived");
  }
};

int main() {
  Derived derived;
  derived.foo(3.4f);
  return 0;
}

You would expect that this prints `Base`. But since the lookup happens on `Derived`, it finds a matching `Derived::foo(int e)` and applies that one (we print "Derived") even though we intended to call `Base::foo()`. By adding `using Base::foo`, we make the overloaded version available in `Derived` and pick that one instead when running `main()` (we print "Base", as anticipated).

In my example, it would not be a problem. But I guess the warning is here to just generally warn the user if the code was intended or just a typo resulting in unexpected behavior.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21161#discussion_r1780565499