Skip to content

fix: merge namespaces when collecting results #896

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.

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

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Apr 5, 2025

I'm not 100% sure if this is the correct approach. I would guess that something like this already happens somewhere, but I couldn't find anything.

When using multiple sources (→ multiple extraction jobs), then only the first one will be "visible", because its global namespace/global symbol will be the only one that gets merged with info_.merge(info). All other global namespaces will be discarded, because there's already one present. Their members will still be in the corpus, but there's no path from the global symbol to them.

Because of ODR, only namespaces have to be merged.

@cppalliance-bot
Copy link

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

@alandefreitas
Copy link
Collaborator

This looks very weird to me.

When using multiple sources (→ multiple extraction jobs), then only the first one will be "visible"

If by "visible" you mean copied into info_, then no. info_.merge(info); is called on all infos for all TUs. Any new symbols will be added there. The loop is only for symbols that couldn't be included in the info_ because they're duplicates. Then we have the merge algorithm for this.

its global namespace/global symbol will be the only one that gets merged with info_.merge(info)

Not really. The second, third, ... is supposed to be merged with merge(**it, std::move(*other));.

Their members will still be in the corpus, but there's no path from the global symbol to them.

merge(**it, std::move(*other)); is supposed to create this "path" (include the members in the well... members of the namespace). Even if merge(**it, std::move(*other)); didn't do that, I'm not sure how this casting would change things unless the implementation of merge is wrong.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Apr 6, 2025

Even if merge(**it, std::move(*other)); didn't do that, I'm not sure how this casting would change things unless the implementation of merge is wrong.

Then the implementation of merge might be wrong. In the execution context, merge(Info& I, Info&& Other) is called, which doesn't perform any downcasting, it only upcasts to SourceInfo. merge(NamespaceInfo& I, NamespaceInfo&& Other) is never called from there. In fact, it would merge the infos (if called).

I think the intention was to call the most specific merge function. One could either turn merge into a virtual function or use visit:

visit(**it, [&]<typename T>(T& target) {
    auto *source = dynamic_cast<T*>(other.get());
    MRDOCS_ASSERT(source);
    merge(target, std::move(*source));
});

^ this might be the intention here.

@alandefreitas
Copy link
Collaborator

Exactly!

@Nerixyz Nerixyz force-pushed the fix/merge-namespaces branch from 4730ae7 to 13b1fcb Compare April 6, 2025 14:50
@cppalliance-bot
Copy link

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

@alandefreitas
Copy link
Collaborator

Now, this is good to go, right? I'll merge this one first because whatever I merge, the other needs to be rebased, and this one is more important.

@alandefreitas alandefreitas merged commit 1e8d691 into cppalliance:develop Apr 7, 2025
11 checks passed
@Nerixyz Nerixyz deleted the fix/merge-namespaces branch April 7, 2025 17:22
@Nerixyz
Copy link
Contributor Author

Nerixyz commented Apr 7, 2025

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants