Skip to content
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

[cling] Use single Parser for LookupHelper #17353

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Jan 6, 2025

It is the only construction of a temporary parser, and it seems not necessary (anymore).


This will allow us to drop some downstream Clang patches; not included yet, will need some sequencing with other changes in our monorepo fork. Let's see what the CI has to say first 🙃

@hahnjo hahnjo added the in:Cling label Jan 6, 2025
@hahnjo hahnjo requested review from vgvassilev and dpiparo January 6, 2025 10:07
@hahnjo hahnjo self-assigned this Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

Test Results

    17 files      17 suites   4d 3h 3m 28s ⏱️
 2 680 tests  2 680 ✅ 0 💤 0 ❌
43 906 runs  43 906 ✅ 0 💤 0 ❌

Results for commit 399c950.

♻️ This comment has been updated with latest results.

@vgvassilev
Copy link
Member

Probably worth checking with cmssw.

@hahnjo
Copy link
Member Author

hahnjo commented Jan 6, 2025

Probably worth checking with cmssw.

Yes, good idea, first wanted to see if our own CI agrees with my local testing. I'll allow CMS people to come back from vacation first and also update to ROOT master before asking.

It is the only construction of a temporary parser, and it seems not
necessary (anymore).
@hahnjo hahnjo force-pushed the cling-temporary-parsers branch from 71333a4 to 399c950 Compare January 6, 2025 14:54
@pcanal
Copy link
Member

pcanal commented Jan 8, 2025

The original reason for the separate parser (as seen in its introduction: b1003a1) is to side-steps potential failures/bugs in the parser error recovery.

@hahnjo
Copy link
Member Author

hahnjo commented Jan 9, 2025

Ok, but I'm not sure if a second parser is actually sufficient for that: In case of problems with the parser error recovery, the main parser for Cling is untouched and continues to work, but now the single parser used by the LookupHelper is corrupt and future queries to it will fail for unrelated reasons. If we created a new temporary parser for every lookup, the story would be different, but with the current state what am I missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants