Skip to content

[cling] Always add primary NamespaceDecl to fNSSet #23024

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
jase231 wants to merge 1 commit into
base: master
Choose a base branch
from
+10 −0
Open

[cling] Always add primary NamespaceDecl to fNSSet #23024

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions core/metacling/src/TCling.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5872,6 +5872,16 @@ namespace {

nsDecl->setHasExternalVisibleStorage();
fNSSet.insert(nsDecl);

// When cling eventually queries fNSSet/fNSFromRootmaps, it always does so
// using the primary DeclContext. Therefore we need to store

@pcanal pcanal Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to record both?

@jase231 jase231 Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As in, "why do we need to record both rather than only recording the primary"? I don't think we do; if you think so too I will change to always recording the primary. I kept both because I didn't want to potentially break anything by dropping the non-primary in case that was used elsewhere, but I haven't found any other such case. I'll investigate the test failures this week. I'll also try to reproduce the original issue in a standard ROOT build, since I initially ran into this issue while working on wheels.

Also, based on getPrimaryContext's definition I think the return should always be non-null i.e. I can just cast, if you agree.

@pcanal pcanal Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am confused. I read:

"why do we need to record both rather than only recording the primary"? I don't think we do
I don't think we do;
I kept both because ....

Where the last 2 statement seem contradictory ...

@jase231 jase231 Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to be conservative in my change since I'm inexperienced in this part of the codebase, so rather than completely stop inserting non-primary decls, I decided to record both to avoid risking

to potentially break anything by dropping the non-primary in case that was used elsewhere

So recording both was out of abundance of caution. If you think recording non-primaries is pointless, then I will stop doing so, but I wasn't confident enough in my understanding to be sure that recording non-primaries is pointless.

@pcanal pcanal Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not know for sure whether both are needed. The comment however is pretty assertive: "When cling ... queries ... it always does so using the primary DeclContext", so I assumed the non-primary was unnecessary.

Either way, storing both if it is not needed would both cost 'unnecessary memory use' and increase the challenge in understanding the code (eg make wrong assumption of what is needed or not).

I.e. I strongly recommend that we investigate whether or not they are both needed (ideally both via testing and understanding the usage of the collection).

Thanks.

// the namespace's primary, not just nsDecl which might just
// be a non-primary declaration context for the namespace.
auto *primaryNsDecl = dyn_cast_or_null<NamespaceDecl>(nsDecl->getPrimaryContext());
if (primaryNsDecl && primaryNsDecl != nsDecl) {
primaryNsDecl->setHasExternalVisibleStorage();
fNSSet.insert(primaryNsDecl);
}
return true;
}
bool VisitClassTemplateSpecializationDecl(ClassTemplateSpecializationDecl* specDecl) {
Expand Down
Loading