-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core] remove class prefix if any in IsDefAlloc #17048
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
|
Thanks for this PR. Would it make sense to also transform in a test (or add to the one written for this PR) the example reported in the original issue ? std::string n;
TClassEdit::GetNormalizedName(n, "std::vector<float, class std::allocator<float>>");
n == std::string("vector<float>"); |
|
@bellenot Do you understand by chance why the build fails on Windows after this change? |
|
@ferdymercury I'll check after the hackathon (in a couple of days...) |
GH issue as suggested by dpiparo
Test Results 19 files 19 suites 3d 20h 33m 19s ⏱️ For more details on these failures, see this check. Results for commit 6ae11aa. ♻️ This comment has been updated with latest results. |
|
|
@ferdymercury I'll debug it and let you know (need a full debug build) |
|
@ferdymercury I found the problem. Trying to find the proper fix. |
|
@ferdymercury maybe not the correct and maybe not the final solution, but can you try this diff (including your change): @pcanal can check/comment on this, but I think this code: is wrong. I.e. |
Your analysis seems correct. However, it also seems that the 'error' is harmless as each use of the |
|
@pcanal it crashes in with |
right, I did indeed mis-read the code. The misplacement is fatal (and surprising it does not cause more trouble ...) |
|
It's solved now! |
bellenot
left a comment
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.
It's solved now!
Congrats! Looks good to me, but I'll let @pcanal give the final green light
as suggested by pcanal
as suggested by pcanal
|
@pcanal do you know if the remaining test failures are related to this PR ? thanks 4 the review. |
|
They shouldn't but they don't usually fail either. Let's rerun the test and see. |
Yes, now it's others which fail, so it seemed random. |
|
All tests passes (after reruns). |
This Pull request:
Changes or fixes:
Fixes #6607
Related: #10749
Sibling roottest PR: root-project/roottest#1237
Checklist: