Skip to content

BaseMembersFinalizer considers the relationship between inherited members and shadowing #1026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

BaseMembersFinalizer considers the relationship between inherited members and shadowing #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0f4891
add test
K-ballo Sep 15, 2025
e822361
fix
K-ballo Sep 15, 2025
458ce17
adjust inherit-base-members tests
K-ballo Sep 15, 2025
7c34ba6
Merge branch 'develop' into fix-1025
alandefreitas Sep 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions src/lib/Metadata/Finalizers/BaseMembersFinalizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,53 +30,57 @@ inheritBaseMembers(
RecordInterface const& base,
AccessKind const A)
{
auto&& members = allMembers(derived);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if (A == AccessKind::Public)
{
// When a class uses public member access specifier to derive from a
// base, all public members of the base class are accessible as public
// members of the derived class and all protected members of the base
// class are accessible as protected members of the derived class.
// Private members of the base are never accessible unless friended.
inheritBaseMembers(derivedId, derived.Public, base.Public);
inheritBaseMembers(derivedId, derived.Protected, base.Protected);
inheritBaseMembers(derivedId, derived.Public, base.Public, members);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

inheritBaseMembers(derivedId, derived.Protected, base.Protected, members);
}
else if (A == AccessKind::Protected)
{
// When a class uses protected member access specifier to derive from a
// base, all public and protected members of the base class are
// accessible as protected members of the derived class (private members
// of the base are never accessible unless friended).
inheritBaseMembers(derivedId, derived.Protected, base.Public);
inheritBaseMembers(derivedId, derived.Protected, base.Protected);
inheritBaseMembers(derivedId, derived.Protected, base.Public, members);
inheritBaseMembers(derivedId, derived.Protected, base.Protected, members);
}
else if (A == AccessKind::Private && corpus_.config->extractPrivate)
{
// When a class uses private member access specifier to derive from a
// base, all public and protected members of the base class are
// accessible as private members of the derived class (private members
// of the base are never accessible unless friended).
inheritBaseMembers(derivedId, derived.Private, base.Public);
inheritBaseMembers(derivedId, derived.Private, base.Protected);
inheritBaseMembers(derivedId, derived.Private, base.Public, members);
inheritBaseMembers(derivedId, derived.Private, base.Protected, members);
}
}

template <std::ranges::range Range>
void
BaseMembersFinalizer::
inheritBaseMembers(
SymbolID const& derivedId,
RecordTranche& derived,
RecordTranche const& base)
RecordTranche const& base,
Range allMembers)
{
inheritBaseMembers(derivedId, derived.NamespaceAliases, base.NamespaceAliases);
inheritBaseMembers(derivedId, derived.Typedefs, base.Typedefs);
inheritBaseMembers(derivedId, derived.Records, base.Records);
inheritBaseMembers(derivedId, derived.Enums, base.Enums);
inheritBaseMembers(derivedId, derived.Functions, base.Functions);
inheritBaseMembers(derivedId, derived.StaticFunctions, base.StaticFunctions);
inheritBaseMembers(derivedId, derived.Variables, base.Variables);
inheritBaseMembers(derivedId, derived.StaticVariables, base.StaticVariables);
inheritBaseMembers(derivedId, derived.Concepts, base.Concepts);
inheritBaseMembers(derivedId, derived.Guides, base.Guides);
inheritBaseMembers(derivedId, derived.NamespaceAliases, base.NamespaceAliases, allMembers);
inheritBaseMembers(derivedId, derived.Typedefs, base.Typedefs, allMembers);
inheritBaseMembers(derivedId, derived.Records, base.Records, allMembers);
inheritBaseMembers(derivedId, derived.Enums, base.Enums, allMembers);
inheritBaseMembers(derivedId, derived.Functions, base.Functions, allMembers);
inheritBaseMembers(derivedId, derived.StaticFunctions, base.StaticFunctions, allMembers);
inheritBaseMembers(derivedId, derived.Variables, base.Variables, allMembers);
inheritBaseMembers(derivedId, derived.StaticVariables, base.StaticVariables, allMembers);
inheritBaseMembers(derivedId, derived.Concepts, base.Concepts, allMembers);
inheritBaseMembers(derivedId, derived.Guides, base.Guides, allMembers);
}

namespace {
Expand All @@ -91,12 +95,14 @@ shouldCopy(Config const& config, Info const& M)
}
}

template <std::ranges::range Range>
void
BaseMembersFinalizer::
inheritBaseMembers(
SymbolID const& derivedId,
std::vector<SymbolID>& derived,
std::vector<SymbolID> const& base)
std::vector<SymbolID> const& base,
Range allMembers)
{
for (SymbolID const& otherID: base)
{
Expand All @@ -116,7 +122,7 @@ inheritBaseMembers(

// Check if derived class has a member that shadows the base member
auto shadowIt = std::ranges::find_if(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

derived,
allMembers,
[&](SymbolID const& id)
{
Info* infoPtr = corpus_.find(id);
Expand All @@ -135,7 +141,7 @@ inheritBaseMembers(
// are the same
return info.Name == otherInfo.Name;
});
MRDOCS_CHECK_OR_CONTINUE(shadowIt == derived.end());
MRDOCS_CHECK_OR_CONTINUE(shadowIt == allMembers.end());

// Not a shadow, so inherit the base member
if (!shouldCopy(corpus_.config, otherInfo))
Expand Down
10 changes: 7 additions & 3 deletions src/lib/Metadata/Finalizers/BaseMembersFinalizer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,22 @@ class BaseMembersFinalizer
RecordInterface& derived,
RecordInterface const& base,
AccessKind A);


template <std::ranges::range Range>
void
inheritBaseMembers(
SymbolID const& derivedId,
RecordTranche& derived,
RecordTranche const& base);
RecordTranche const& base,
Range allMembers);

template <std::ranges::range Range>
void
inheritBaseMembers(
SymbolID const& derivedId,
std::vector<SymbolID>& derived,
std::vector<SymbolID> const& base);
std::vector<SymbolID> const& base,
Range allMembers);

void
finalizeRecords(std::vector<SymbolID> const& ids);
Expand Down
6 changes: 0 additions & 6 deletions test-files/golden-tests/config/inherit-base-members/copy-dependencies.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,6 @@ class derived
| This function should be inherited by derived classes&period;
| link:#base-do_base_shadowed[`do&lowbar;base&lowbar;shadowed`]
| This function should shadow the excluded&lowbar;base function&period;
| link:#base-do_derived_shadowed[`do&lowbar;derived&lowbar;shadowed`]
| This function should be shadowed by derived classes&period;
| link:#derived-do_excluded_inherited[`do&lowbar;excluded&lowbar;inherited`]
| This function should be inherited by derived classes&period;
| link:#derived-do_shadowed[`do&lowbar;shadowed`]
Expand Down Expand Up @@ -546,16 +544,12 @@ class protected&lowbar;derived
| This function should be inherited by derived classes&period;
| link:#base-base_shadowed[`base&lowbar;shadowed`]
| This function should shadow the excluded&lowbar;base function&period;
| link:#base-derived_shadowed[`derived&lowbar;shadowed`]
| This function should be shadowed by derived classes&period;
| link:#base_base-do_base_base_inherited[`do&lowbar;base&lowbar;base&lowbar;inherited`]
| This function should be indirectly inherited by derived classes&period;
| link:#base-do_base_inherited[`do&lowbar;base&lowbar;inherited`]
| This function should be inherited by derived classes&period;
| link:#base-do_base_shadowed[`do&lowbar;base&lowbar;shadowed`]
| This function should shadow the excluded&lowbar;base function&period;
| link:#base-do_derived_shadowed[`do&lowbar;derived&lowbar;shadowed`]
| This function should be shadowed by derived classes&period;
| link:#protected_derived-do_excluded_inherited[`do&lowbar;excluded&lowbar;inherited`]
| This function should be inherited by derived classes&period;
| link:#protected_derived-do_shadowed[`do&lowbar;shadowed`]
Expand Down
3 changes: 0 additions & 3 deletions test-files/golden-tests/config/inherit-base-members/copy-dependencies.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,6 @@ <h2>Protected Member Functions</h2>
<tr>
<td><a href="#base-do_base_inherited"><code>do_base_inherited</code></a> </td><td><span>This function should be inherited by derived classes.</span></td></tr><tr>
<td><a href="#base-do_base_shadowed"><code>do_base_shadowed</code></a> </td><td><span>This function should shadow the excluded_base function.</span></td></tr><tr>
<td><a href="#base-do_derived_shadowed"><code>do_derived_shadowed</code></a> </td><td><span>This function should be shadowed by derived classes.</span></td></tr><tr>
<td><a href="#derived-do_excluded_inherited"><code>do_excluded_inherited</code></a> </td><td><span>This function should be inherited by derived classes.</span></td></tr><tr>
<td><a href="#derived-do_shadowed"><code>do_shadowed</code></a> </td><td><span>This function should be shadowed by derived classes.</span></td></tr>
</tbody>
Expand Down Expand Up @@ -690,11 +689,9 @@ <h2>Protected Member Functions</h2>
<td><a href="#base_base-base_base_inherited"><code>base_base_inherited</code></a> </td><td><span>This function should be indirectly inherited by derived classes.</span></td></tr><tr>
<td><a href="#base-base_inherited"><code>base_inherited</code></a> </td><td><span>This function should be inherited by derived classes.</span></td></tr><tr>
<td><a href="#base-base_shadowed"><code>base_shadowed</code></a> </td><td><span>This function should shadow the excluded_base function.</span></td></tr><tr>
<td><a href="#base-derived_shadowed"><code>derived_shadowed</code></a> </td><td><span>This function should be shadowed by derived classes.</span></td></tr><tr>
<td><a href="#base_base-do_base_base_inherited"><code>do_base_base_inherited</code></a> </td><td><span>This function should be indirectly inherited by derived classes.</span></td></tr><tr>
<td><a href="#base-do_base_inherited"><code>do_base_inherited</code></a> </td><td><span>This function should be inherited by derived classes.</span></td></tr><tr>
<td><a href="#base-do_base_shadowed"><code>do_base_shadowed</code></a> </td><td><span>This function should shadow the excluded_base function.</span></td></tr><tr>
<td><a href="#base-do_derived_shadowed"><code>do_derived_shadowed</code></a> </td><td><span>This function should be shadowed by derived classes.</span></td></tr><tr>
<td><a href="#protected_derived-do_excluded_inherited"><code>do_excluded_inherited</code></a> </td><td><span>This function should be inherited by derived classes.</span></td></tr><tr>
<td><a href="#protected_derived-do_shadowed"><code>do_shadowed</code></a> </td><td><span>This function should be shadowed by derived classes.</span></td></tr><tr>
<td><a href="#protected_derived-excluded_inherited"><code>excluded_inherited</code></a> </td><td><span>This function should be inherited by derived classes.</span></td></tr>
Expand Down
48 changes: 0 additions & 48 deletions test-files/golden-tests/config/inherit-base-members/copy-dependencies.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,22 +335,6 @@
</returns>
</doc>
</function>
<function name="do_derived_shadowed" access="protected" id="KGOrnDVJV2nPDK1hlkvBm6diYXY=">
<file short-path="copy-dependencies.cpp" source-path="copy-dependencies.cpp" line="42"/>
<return>
<type class="lvalue-reference">
<pointee-type id="sk8HTAQHhzXfZej4IJliADonAJQ=" name="base"/>
</type>
</return>
<doc>
<brief>
<text>This function should be shadowed by derived classes.</text>
</brief>
<returns>
<text>A base class to test inheritance and shadowing</text>
</returns>
</doc>
</function>
<function name="do_excluded_inherited" access="protected" id="oW3pQYXjjv2wrXIwIceF6Xd8nTA=">
<file short-path="copy-dependencies.cpp" source-path="copy-dependencies.cpp" line="16"/>
<return>
Expand Down Expand Up @@ -517,22 +501,6 @@
</returns>
</doc>
</function>
<function name="derived_shadowed" id="aYhSUMJkgCRo3AZmBb/rre04Ghg=">
<file short-path="copy-dependencies.cpp" source-path="copy-dependencies.cpp" line="35"/>
<return>
<type class="lvalue-reference">
<pointee-type id="sk8HTAQHhzXfZej4IJliADonAJQ=" name="base"/>
</type>
</return>
<doc>
<brief>
<text>This function should be shadowed by derived classes.</text>
</brief>
<returns>
<text>A base class to test inheritance and shadowing</text>
</returns>
</doc>
</function>
<function name="do_base_base_inherited" id="MEPVWyGd8KdZFR1kBYlQMgPyQlo=">
<file short-path="copy-dependencies.cpp" source-path="copy-dependencies.cpp" line="26"/>
<return>
Expand Down Expand Up @@ -581,22 +549,6 @@
</returns>
</doc>
</function>
<function name="do_derived_shadowed" access="protected" id="KGOrnDVJV2nPDK1hlkvBm6diYXY=">
<file short-path="copy-dependencies.cpp" source-path="copy-dependencies.cpp" line="42"/>
<return>
<type class="lvalue-reference">
<pointee-type id="sk8HTAQHhzXfZej4IJliADonAJQ=" name="base"/>
</type>
</return>
<doc>
<brief>
<text>This function should be shadowed by derived classes.</text>
</brief>
<returns>
<text>A base class to test inheritance and shadowing</text>
</returns>
</doc>
</function>
<function name="do_excluded_inherited" access="protected" id="KdkXQxu2xOfAXegJgCMmLcFq8MM=">
<file short-path="copy-dependencies.cpp" source-path="copy-dependencies.cpp" line="16"/>
<return>
Expand Down
Loading
Loading