-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[TCling] Do not add decls for control statements if already annotated #15368
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
Test Results 17 files 17 suites 3d 13h 21m 43s ⏱️ For more details on these failures, see this check. Results for commit 1c45df8. ♻️ This comment has been updated with latest results. |
978cafb to
1b3bab6
Compare
|
Not a proper solution. What we ideally want to do is check if a |
vgvassilev
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.
Any chance in adding a test?
|
I also think a test would be good. @vgvassilev in which subdirectory would you put the simple macro which was crashing in https://github.com/root-project/roottest/tree/master/cling ? |
|
Maybe we can do a death test in the test folder of core/metacling. |
9b0507a to
11ba471
Compare
|
rebased & test added |
11ba471 to
c97b32c
Compare
dpiparo
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.
LGTM: thanks a lot for this fix. Once the tests are green, afaik we are ready to merge.
c97b32c to
04339e5
Compare
|
I changed the test. Before we were also testing the "line" number according to the interpreter, which can be platform dependent. Now we are independent from that |
|
Can it be that this fix fixes also #15537 ? That's what I see, I think... |
|
As a note, once we remove |
vgvassilev
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.
Lgtm!
Still fails for me. It seems important to put |
04339e5 to
ce44a75
Compare
ce44a75 to
0d357e3
Compare
0d357e3 to
1c45df8
Compare
hahnjo
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.
Thanks!
This Pull request:
Changes or fixes:
Checklist:
This PR fixes #8367