-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Reimplement HandleInvalidTagDeclDefinition without clang support. #6770
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
Reimplement HandleInvalidTagDeclDefinition without clang support. #6770
Conversation
|
Starting build on |
362df91 to
eaa7771
Compare
|
Starting build on |
|
Build failed on mac1015/cxx17. Errors:
|
|
Build failed on ROOT-fedora30/cxx14. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-fedora31/noimt. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
eaa7771 to
c61fb24
Compare
|
Starting build on |
|
Build failed on mac1014/python3. Failing tests: |
|
@phsft-bot build! |
|
Starting build on |
|
Build failed on windows10/cxx14. |
|
Build failed on mac11.0/cxx17. Failing tests: |
|
@phsft-bot build! |
|
Starting build on |
c61fb24 to
bcf340e
Compare
|
Starting build on |
|
Build failed on ROOT-fedora30/cxx14. Errors:
|
|
Build failed on ROOT-fedora31/noimt. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on mac11.0/cxx17. Errors:
|
|
Build failed on mac1014/python3. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. |
|
@phsft-bot build! |
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. |
|
Looks like we one of the failures is due to infrastructure and the other fails elsewhere too. |
|
@phsft-bot build! |
|
Starting build on |
|
ping... |
|
I suppose you're pinging me here? It's of much greater success to ping me during work days :-) I'll try to review this week; the release needs to go first. |
|
There is not much to review here but ok. |
oshadura
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.
For me, it looks like it is a missing upstreamed patch in LLVM and two reverts of existing comments...
oshadura
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.
Ops, I wanted to approve instead
|
Thanks! @Axel-Naumann, let us know if you have comments. |
|
@phsft-bot build! |
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
And 8 more Warnings:
|
Should fix the warnings visible e.g. [here](root-project#6770 (comment)) and [here](root-project#8070 (comment)).
Should fix the warnings visible e.g. [here](root-project#6770 (comment)) and [here](root-project#8070 (comment)).
Should fix the warnings visible e.g. [here](root-project#6770 (comment)) and [here](root-project#8070 (comment)).
Should fix the warnings visible e.g. [here](root-project#6770 (comment)) and [here](root-project#8070 (comment)).
Should fix the warnings visible e.g. [here](#6770 (comment)) and [here](#8070 (comment)).
|
Looks like the review by @oshadura missed a removed code branch. Was that removed intentionally? |
|
The one in clang or in cling? I think they are both intentional. |
Axel-Naumann
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.
Sorry, failed to actually submit the review. Here it is. And the removed code branch still exists in llvm's main, so I'd like to have an explanation why it was removed here.
| // in constant evaluation. | ||
| if (TSK == TSK_ExplicitInstantiationDefinition) | ||
| MarkVTableUsed(PointOfInstantiation, Instantiation, true); | ||
| else if (MightHaveConstexprVirtualFunctions) |
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.
This seems to have been lost?
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.
ping @vgvassilev this still needs to be addressed!
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.
...and I don't know how to make sure it will be addressed (rather than just forgotten). Maybe open an issue, @vgvassilev ?
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.
Yeah, indeed that was missing. It is readded in #8135
Luckily, being patch-free that would have been 'transiently' fixed with the next upgrade.
No description provided.