[cling] Do not wrap overloaded operator function declarations
#11265
Conversation
|
Can one of the admins verify this patch? |
|
@phsft-bot build |
|
Starting build on |
|
Build failed on windows10/cxx14. |
|
@jiangyilism Thanks for the pull request; I'll review soon. I had an issue open for this since Dec 2021 (#9449) |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
And 60 more Failing tests:
|
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
Warnings:
And 130 more Failing tests:
And 7 more |
|
Build failed on mac12/noimt. Warnings:
And 53 more Failing tests:
|
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
And 54 more Failing tests:
|
|
Build failed on mac11/cxx14. Errors:
Warnings:
And 54 more Failing tests:
And 7 more |
|
@phsft-bot build |
|
Starting build on |
jalopezg-git
changed the title
operator function declarations
|
Build failed on mac11/cxx14. Failing tests: |
Axel-Naumann
requested review from
vgvassilev
and removed request for
Axel-Naumann
There was a problem hiding this comment.
Looks reasonable to me. Does this pass cling's testsuite?
FWIW, we have landed a patch in clang in that area - https://reviews.llvm.org/D127284 which should be backportable but might need more work to properly adopt in cling.
There was a problem hiding this comment.
Many thanks for the contribution, @jiangyilism! 👍 I have attached a few suggestions.
Also, does the patch pass the cling test suite? Additionally, we would need to add a few tests cases for this (namely those in the pull request description) -- those should be placed in the interpreter/cling/test/ directory.
Are you up for it? 🙂
Thanks for the review. I will address the issues and add test cases in |
|
@jiangyilism, any progress in this regard? |
|
Any progress? I am college teacher and I can't teach student to overload cout<<Object in cling due to this issue! |
|
Many thanks.
Jiang Yi ***@***.***> 于 2023年6月5日周一 05:15写道：
… @jalopezg-git <https://github.com/jalopezg-git> Sorry for the late reply.
I am still trying to fix some corner cases while avoiding too intrusive
changes. I will update the pull request soon.
—
Reply to this email directly, view it on GitHub
<#11265 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACN3SSVEF3GLBPRSV2CHAELXJWPUNANCNFSM573UE76Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
jiangyilism
force-pushed
the
enhance_support_of_cling_cmd_prompt_func_defining
branch
from
b34a511
to
aa539b6
Compare
jiangyilism
force-pushed
the
enhance_support_of_cling_cmd_prompt_func_defining
branch
from
aa539b6
to
5c9ef9f
Compare
|
SdtElectronics/cline#10 and root-project/cling#490 redirect the discussion of parsing |
Many thanks @jiangyilism. Please, re-request a code review when you think it is ready 🙂! |
It is ready for review. However I have no clue about the failure test report above. clang-format failed to checkout code before running. The windows test fail seems to be file permission error irrelevant to this pull request. The Fedora test fail is unreproducible on my ubuntu either. |
|
@phsft-bot build |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Errors:
|
There was a problem hiding this comment.
Good work, @jiangyilism, (and many thanks for the contribution, much appreciated 🙂)!
I have attached a few comments to be discussed before merging. Does it pass also the other tests in the cling test suite?
@vgvassilev What do you think? Do you have any further comments? I think this is mostly okay to go.
|
Build failed on mac12arm/cxx20. Failing tests:
|
…d prompt
e.g. class_a::class_a() = default;
class_a::~class_a();
They have no function definition body so caller of IsClassOrFunction()
should not try to parse their function bodies.
e.g. ::class_a class_b::mem_func_b() { return 'w'; }
skip :: right before class_a while calculating wrap point
Fixes root-project#9449 so it is possible to define operator overload in cling cmd prompt. btw, Make SkipPointerRefs() to not assume next token being a tok::raw_identifier since it can be a global-scoped identifier, e.g. int* ::class_a::func_b(short c) { return nullptr; }
jiangyilism
force-pushed
the
enhance_support_of_cling_cmd_prompt_func_defining
branch
from
5c9ef9f
to
ba17dc4
Compare
|
@phsft-bot build |
|
Starting build on |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
There was a problem hiding this comment.
In principle, LGTM, although the logic in MinimalPPLexer::IsClassOrFunction() is getting convoluted / hard to maintain.
The cling test suite passes, but let's also wait for the ROOT CI results 🙂.
@vgvassilev any concerns about merging this patch?
|
Build failed on mac11/noimt. Failing tests: |
|
@phsft-bot build |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. |
|
Build failed on windows10/cxx14. Failing tests: |
|
Build failed on mac11/noimt. Failing tests: |
Changes or fixes:
Let
a_tbe a c++ class, and we have doneusing std::ostream;.In standalone cling cmd prompt, all of the below function definitions fail to be parsed and compiled(incrementally).
They get "error: function definition is not allowed here" due to
cling::utils::getWrapPoint()wrongly determines to wrap function definition inside wrapper function__cling_Un1Qu3.... The root cause is thatMinimalPPLexer::IsClassOrFunction()does not parse them as function definitions. This pullrequest enablesMinimalPPLexer::IsClassOrFunction()to parse these kinds of func def but there are probably more edge cases of standard c++ func def not being recognized byMinimalPPLexer::IsClassOrFunction().Checklist:
This PR fixes "error: function definition is not allowed here" ( root-project/cling#184 ).