Skip to content

[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

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

devajithvs
Copy link
Contributor

This Pull request:

Changes or fixes:

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #8367

Copy link

github-actions bot commented Apr 28, 2024

Test Results

    17 files      17 suites   3d 13h 21m 43s ⏱️
 2 663 tests  2 661 ✅ 0 💤 2 ❌
43 575 runs  43 573 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 1c45df8.

♻️ This comment has been updated with latest results.

@devajithvs devajithvs force-pushed the dev.fix_8367 branch 2 times, most recently from 978cafb to 1b3bab6 Compare April 28, 2024 17:16
@devajithvs devajithvs changed the title [TCling] Do not resolve control statements at runtime [TCling] Do not add decls for control statements if already annotated Apr 28, 2024
@devajithvs
Copy link
Contributor Author

Not a proper solution.

What we ideally want to do is check if a VarDecl with the same identifier is already added for the current ControlScope.

Copy link
Member

@vgvassilev vgvassilev left a 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?

@dpiparo
Copy link
Member

dpiparo commented Apr 29, 2024

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 ?

@vgvassilev
Copy link
Member

Maybe we can do a death test in the test folder of core/metacling.

@dpiparo
Copy link
Member

dpiparo commented Sep 25, 2024

rebased & test added

Copy link
Member

@dpiparo dpiparo left a 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.

@dpiparo
Copy link
Member

dpiparo commented Sep 25, 2024

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

@dpiparo dpiparo closed this Sep 25, 2024
@dpiparo dpiparo reopened this Sep 25, 2024
@dpiparo
Copy link
Member

dpiparo commented Sep 26, 2024

Can it be that this fix fixes also #15537 ? That's what I see, I think...

@devajithvs
Copy link
Contributor Author

devajithvs commented Sep 26, 2024

As a note, once we remove auto auto support in: #16410, this will no longer be needed and will need to be reverted.

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm!

@hahnjo
Copy link
Member

hahnjo commented Sep 26, 2024

Can it be that this fix fixes also #15537 ? That's what I see, I think...

Still fails for me. It seems important to put badcast into a macro...

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

Thanks!

@devajithvs devajithvs merged commit 8f63509 into root-project:master Oct 31, 2024
19 of 21 checks passed
@devajithvs devajithvs deleted the dev.fix_8367 branch October 31, 2024 12:41
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.

*** Break *** segmentation violation in case of compilation errors in unnamed macros
4 participants