BaseMembersFinalizer considers the relationship between inherited members and shadowing

[K-ballo]

fixes #1025

[cppalliance-bot]

An automated preview of the documentation is available at https://1026.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://1026.mrdocs.prtest2.cppalliance.org/index.html

[K-ballo]

Note: extract-private is necessary or else the corpus doesn't have the required information to properly resolve name hiding

[cppalliance-bot]

An automated preview of the documentation is available at https://1026.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

Isn't members a value type with const-reference semantics? Is this a reference to a temporary we're relying on the compiler to keep alive?

If so, wouldn't

auto members = allMembers(derived);

mean the same thing here since this is just a view?

[alandefreitas]

Is this safe? members is a const-view of the derived members, but the function is changing the set of members. It's OK to ask for all members there again, but this is supposed to be a lazy operation (unless there's some bug somewhere else).

[alandefreitas]

Oh... Here's the relevant point. Yes. We can just call "allMembers" here to get all members of derived. This should be a lazy operation so there's no extra non-constant cost in just calling it again.

The only unnecessary extra-cost we have here is the cost of iterating all kinds of members while we want the functions in different access modes. We can fix that by just creating a lazy range that only contains the functions, or by iterating the functions for each access mode.

[alandefreitas]

This is great.

I just checked how the example in #1025 behaves:

https://godbolt.org/z/PbfEsTq5b

Y makes bar() protected, and the inherited version is not there. The code we have right now wouldn't check this interaction. And foo() just works as expected.

Something to think about is the use of the term' shadowing.' I don't know if there's a good solution to that, but there's another issue: the relationship between these inherited members and using decl shadows. We've been using the same term for two different things. When we work on this other issue, we would be doing it in the code and it could be confusing.

You probably already did this, but please also check if there's any interaction between this solution and the option regarding how these members should be inherited (config->inheritBaseMembers).

[alandefreitas]

Note: extract-private is necessary or else the corpus doesn't have the required information to properly resolve name hiding

Oh... That's very interesting. So we want the information about inheritance to be correct regardless of what the user chose for extract-private. Still, we also need information about extract-private in the derived class, so we know what to exclude from the base classes potentially. And ASTVisitor attempts to exclude these preemptively.

There's a solution to that. In this case, we can extract these private entities either way. We currently use config->extractPrivate to decide if we're going to extract it. Instead, we should use config->extractPrivate to determine if we're going to extract this private function as a regular or dependency symbol. So before we extract it, we create one of those scope variables, setting it to dependency if that's on or keeping it in the current mode if it's off.