Skip to content

[meta] Ctor name lookup is not enough for the class info - #23098

Open
dpiparo wants to merge 2 commits into
root-project:masterfrom
dpiparo:fix_ROOT10311
Open

[meta] Ctor name lookup is not enough for the class info#23098
dpiparo wants to merge 2 commits into
root-project:masterfrom
dpiparo:fix_ROOT10311

Conversation

@dpiparo

@dpiparo dpiparo commented Aug 18, 2026

Copy link
Copy Markdown
Member

The mechanism in place to allow the initialisation of TClingClassInfo based on names of forward declared classes was a bit too loose. It also allowed for finding a class by the name of its constructor.

The new mechanism is more explicit about the properties of the CXXRecordDecl identified by the lookup (if any).

Fixes ROOT-10311

@dpiparo dpiparo self-assigned this Aug 18, 2026
Comment thread core/metacling/src/TClingClassInfo.cxx Outdated
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 16h 24m 29s ⏱️
 3 860 tests  3 859 ✅ 0 💤 1 ❌
78 638 runs  78 634 ✅ 2 💤 2 ❌

For more details on these failures, see this check.

Results for commit b51209a.

♻️ This comment has been updated with latest results.

@dpiparo
dpiparo marked this pull request as ready for review August 20, 2026 06:11
@dpiparo
dpiparo requested review from aaronj0 and hahnjo August 20, 2026 06:11
the mechanism in place to allow the initialisation of TClingClassInfo
based on names of forward declared classes was a bit too loose. It
also allowed for finding a class by the name of its constructor.

Fixes ROOT-10311
if (const auto *TD = type->getAsTagDecl()) {
decl = TD;
const auto *CXXRD = type->getAsCXXRecordDecl();
if (CXXRD && !CXXRD->hasDefinition()) {

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.

Suggested change
if (CXXRD && !CXXRD->hasDefinition()) {
if (CXXRD && CXXRD->NamedDecl::isUnconditionallyVisible()) {

However, it may be actually more semantically correct, to move this check into quickFindDecl possibly around line 226:

   226 	        next = utils::Lookup::Named(&S, declName.substr(last, c - last), sofar);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if (!decl && type) {
if (const auto *TD = type->getAsTagDecl()) {
decl = TD;
const auto *CXXRD = type->getAsCXXRecordDecl();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious since this dropped getAsTagDecl I assume something like:

gInterpreter->Declare("enum class E : int;");
gInterpreter->ClassInfo_Factory("E");

would not work. Why not just check for a definition on the TagDecl instead? as I think that would still reject TTree::TTree:

if (const auto *TD = type->getAsTagDecl(); TD && !TD->getDefinition())
   decl = TD;

but perhaps I'm missing another reason why we should restrict on CXXRecordDecl

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The choice of CXXRecordDecl is solely this commit 842767f#diff-fb8b9374813e714b3d3b31ba080d2c8121fc3f3882ac775a6cd0904e3a2e882aR102-R103 .
All that code seems to be there to get a grip onto class fwd declarations. TagDecls are a way to achieve that, but it's not always working, and therefore the more stringent check

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