-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[cling] Update policy for GetFullyQualifiedName in cling utils
#19292
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
Conversation
Just like in CppInterOp, set the `SuppressElaboration` and `FullyQualifiedName` policies when getting the fully qualified name of a QualType. Closes the following JIRA ticket: https://its.cern.ch/jira/browse/ROOT-10150
Test Results 20 files 20 suites 3d 4h 50m 49s ⏱️ For more details on these failures, see this check. Results for commit ea1957a. |
| EXPECT_EQ("fHash", hashSubfields[0]->GetFieldName()); | ||
| EXPECT_EQ("std::string", hashSubfields[0]->GetTypeName()); | ||
| EXPECT_EQ("EdmHash<1>::value_type", hashSubfields[0]->GetTypeAlias()); | ||
| EXPECT_EQ("EdmHash::value_type", hashSubfields[0]->GetTypeAlias()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is incorrect: EdmHash is a template class and requires template arguments. Clang stores the inner class outside the template instantiation as an optimization, but the type must have arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it's incorrect :( I realized the printing policies are quite brittle in general, and I can't make all cases work. I'll report on the problems I found soon, in a separate GitHub issue I guess.
| // FIXME: This should really be EdmHash<1>::value_typeT<EdmHash<1>::value_type>, but this is the value we get from | ||
| // TDataMember::GetFullTypeName right now... | ||
| EXPECT_EQ("value_typeT<EdmHash<1>::value_type>", hashSubfields[1]->GetTypeAlias()); | ||
| EXPECT_EQ("EdmHash::value_typeT<EdmHash::value_type>", hashSubfields[1]->GetTypeAlias()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equally wrong, the expected outcome is even documented in the comment.
| const auto &fdesc = desc.GetFieldDescriptor(desc.FindFieldId("m", fooId)); | ||
| EXPECT_EQ(fdesc.GetTypeName(), "std::vector<std::int32_t>"); | ||
| EXPECT_EQ(fdesc.GetTypeAlias(), "MyVec<std::int32_t>"); | ||
| EXPECT_EQ(fdesc.GetTypeAlias(), "CustomStruct::MyVec<std::int32_t>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old version may be correct, depending on how you argue: it's the type alias as the user wrote it, inside of CustomStruct which happens to have a typedef...
|
Related: #999 |
|
Superseeded by the following PR by @devajithvs, coincidentally opened later the same day, but independently: Dev should be the better expert to bring this to conclusion 🙂 |
Just like in CppInterOp, set the
SuppressElaborationandFullyQualifiedNamepolicies when getting the fully qualified name of a QualType.Closes the following JIRA ticket:
https://its.cern.ch/jira/browse/ROOT-10150