[PATCH] D122663: Mark identifier prefixes as substitutable

urnathan added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:6031 +bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) { + NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS); + return mangleSubstitution(reinterpret_cast(NNS));

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:6031 +bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) { + NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS); + return mangleSubstitution(reinterpret_cast(NNS));

[PATCH] D122663: Mark identifier prefixes as substitutable

urnathan added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:6031 +bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) { + NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS); + return mangleSubstitution(reinterpret_cast(NNS));

[PATCH] D122663: Mark identifier prefixes as substitutable

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfed7be096f8e: Mark identifier prefixes as substitutable (authored by hvdijk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3471741 , @erichkeane wrote: > Ping me EOW if @rsmith doesn't respond in the meantime. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122663/new/

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3471960 , @hvdijk wrote: > In D122663#3471866 , @erichkeane > wrote: > >> This actually wouldn't be necessary in this case: cxxfilt is already >> 'right', this is just for

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3471866 , @erichkeane wrote: > This actually wouldn't be necessary in this case: cxxfilt is already 'right', > this is just for humans to be able to see "we changed this from mangling as > to ". That's a good

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. > llvm-cxxfilt has no option to be compatible with earlier incorrect mangling > though, so it would not help with this particular test. But it could help > with other tests, agreed. This actually wouldn't be necessary in this case: cxxfilt is already 'right', this

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3471763 , @erichkeane wrote: > I DID see that and am REASONABLY sure you do, but sometimes folks will ask > for test coverage because they suspect the resulting behavior will > demonstrate some sort of problem with

[PATCH] D122663: Mark identifier prefixes as substitutable

urnathan added a comment. [was out last week] It seems the demangler already gets this right, probably because it just looks like a regular nested name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122663/new/ https://reviews.llvm.org/D122663

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3471748 , @hvdijk wrote: > In D122663#3471741 , @erichkeane > wrote: > >> Ping me EOW if @rsmith doesn't respond in the meantime. It is also not >> clear to me whether

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3471741 , @erichkeane wrote: > Ping me EOW if @rsmith doesn't respond in the meantime. It is also not clear > to me whether you were able to capture/fix the issue he had with the > clang-abi-compat.cpp test. Will

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3470687 , @hvdijk wrote: > In D122663#3457330 , @erichkeane > wrote: > >> LGTM! I would like @rjmccall to take a pass if he ends up having time in >> the next day or two

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3457330 , @erichkeane wrote: > LGTM! I would like @rjmccall to take a pass if he ends up having time in the > next day or two (perhaps tack on an extra day or two because of Easter), else > I'll be willing to

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk updated this revision to Diff 423879. hvdijk added a comment. Fixed release notes to use correct RST syntax. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122663/new/ https://reviews.llvm.org/D122663 Files: clang/docs/ReleaseNotes.rst

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added inline comments. Comment at: clang/docs/ReleaseNotes.rst:240 + GCC. This breaks binary compatibility with code compiled with earlier versions + of clang; use the `-fclang-abi-compat=14` option to get the old mangling. This is incorrect rst

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk marked 2 inline comments as done. hvdijk added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:6031 +bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) { + NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS); + return

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk updated this revision to Diff 423482. hvdijk added a comment. Extend test, add assert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122663/new/ https://reviews.llvm.org/D122663 Files: clang/docs/ReleaseNotes.rst

[PATCH] D122663: Mark identifier prefixes as substitutable

rsmith added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:6031 +bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) { + NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS); + return mangleSubstitution(reinterpret_cast(NNS));

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. LGTM! I would like @rjmccall to take a pass if he ends up having time in the next day or two (perhaps tack on an extra day or two because of Easter), else I'll be willing to approve later in the week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk updated this revision to Diff 423445. hvdijk added a comment. Add to release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122663/new/ https://reviews.llvm.org/D122663 Files: clang/docs/ReleaseNotes.rst

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3457131 , @erichkeane wrote: > Our most recent direction is to document any non-NFC patches in the release > notes if at all sensible, so I think this meets those requirements. Thanks, I'm not finding documentation

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3457130 , @hvdijk wrote: > In D122663#3456507 , @erichkeane > wrote: > >> I believe the answer here is 'yes'. We also likely need release notes. > > Thanks for the

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. In D122663#3456507 , @erichkeane wrote: > I believe the answer here is 'yes'. We also likely need release notes. Thanks for the feedback. -fclang-abi-compat= support added. Other mangling bugfixes appear to not have made it

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk updated this revision to Diff 423426. hvdijk added a comment. Herald added a subscriber: dexonsmith. Allow the old mangling with suitable -fclang-abi-compat= value Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122663/new/

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. @rjmccall is the expert on the Itanium ABI. Code wise, I have no real comments, though he might. As far as: >> Question: does this need to be covered by -fclang-abi-compat= when the prior >> mangling resulted in names that even llvm-cxxfilt agreed made no sense?

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added subscribers: urnathan, erichkeane. hvdijk added a comment. ping. Apologies, I don't know who to add as a reviewer, there is no one specifically listed as code owner and it does not seem to be handled by anyone in particular. @urnathan, @erichkeane, you two appear to be the most

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk added a comment. Question: does this need to be covered by `-fclang-abi-compat=` when the prior mangling resulted in names that even llvm-cxxfilt agreed made no sense? (If it is needed, it should be an easy change.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D122663: Mark identifier prefixes as substitutable

hvdijk created this revision. hvdijk added a reviewer: rsmith. hvdijk added a project: clang. Herald added a project: All. hvdijk requested review of this revision. Herald added a subscriber: cfe-commits. The Itanium C++ ABI says prefixes are substitutable. For most prefixes we already handle