[cling] Use single Parser for LookupHelper

[hahnjo]

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

[github-actions]

Test Results

    18 files      18 suites   4d 2h 40m 23s ⏱️
 2 719 tests  2 719 ✅ 0 💤 0 ❌
47 253 runs  47 253 ✅ 0 💤 0 ❌

Results for commit 3772195.

♻️ This comment has been updated with latest results.

[vgvassilev]

Probably worth checking with cmssw.

[hahnjo]

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.

[pcanal]

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]

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?

[hahnjo]

Hi @smuzaffar, happy new year! I have to start January with another request: When you have time, could you run this PR through CMS testing to make sure it doesn't break things? It will allow us to drop 1.5 patches and make the interpreter more sustainable 😃

[hahnjo]

@smuzaffar could you run this through testing with CMSSW to make sure it doesn't break anything? If I understand correctly, the problems related to ROOT master were resolved and the ROOT6 IB is back.

[smuzaffar]

@hahnjo , cmssw tests are running via cms-sw#217

[smuzaffar]

@hahnjo , cmssw tests look good

[hahnjo]

Thanks for testing and confirming, the additional test coverage is much appreciated!

[hahnjo]

@vgvassilev is this ok in principle? (still need to prepare a tag, I will synchronize with Dev)

[vgvassilev]

@vgvassilev is this ok in principle? (still need to prepare a tag, I will synchronize with Dev)

I am surprised this works. In principle the direction looks good as the "feature" was developed to satisfy root's testsuite and experiments. We probably could move forward.

[hahnjo]

I am surprised this works. In principle the direction looks good as the "feature" was developed to satisfy root's testsuite and experiments. We probably could move forward.

I think it's a consequence of the RAII object cleaning up in case of errors. That already had to work with a single additional Parser for the LookupHelper because if we screwed up there, all future lookups would have failed...

[hahnjo]

This should be ready now, the CI is green

[vgvassilev]

LGTM! Thank you, @hahnjo.