[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

probinson added a comment. > You need to do `returnit`. Doh! Thanks. Those two instantiations could have different function bodies, but would have the same mangled name. Got it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147655/new/

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. In D147655#4650964 , @probinson wrote: > Hi @rsmith, > >> these two different templates would have the same mangling: > > template T returnit() {return I;}; > template T returnit() { return I; } > > I tried compiling `long

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

probinson added a comment. Hi @rsmith, > these two different templates would have the same mangling: template T returnit() {return I;}; template T returnit() { return I; } I tried compiling `long foo() { return returnit(); }` with these two templates, and got different manglings.

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4650666 , @rjmccall wrote: > Yeah, the more I think about this, the more I think that while (1) Apple > should upstream its use of an older default, regardless (2) the existence of > any targets at all with an older

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. Yeah, the more I think about this, the more I think that while (1) Apple should upstream its use of an older default, regardless (2) the existence of any targets at all with an older default means that tests like this always need to be using

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

dyung added a comment. In D147655#4650663 , @rjmccall wrote: > I've been informed that Apple's change to the default ABI compatibility mode > is in our private fork, so this is not your problem; sorry for the noise. > > I'm not sure why we decided to do

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. I've been informed that Apple's change to the default ABI compatibility mode is in our private fork, so this is not your problem; sorry for the noise. I'm not sure why we decided to do that privately; I'll see if we can fix that. Repository: rG LLVM Github Monorepo

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. Hey, Richard. It looks like your new tests are failing on Apple's buildbots, I'm guessing because the default ABI compatibility mode is older. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147655/new/

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4649864 , @dyung wrote: > Hi @rsmith, we have an internal test where your change seems to have changed > the mangling in C++17 mode and wanted to check if that was intentional. [...] > Are these changes in non-C++20

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

dyung added a comment. Hi @rsmith, we have an internal test where your change seems to have changed the mangling in C++17 mode and wanted to check if that was intentional. Consider the following code: // Literals in templates #include template T returnit() {return I;}; enum colour {

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. Ping. Are there any further concerns here? (This obviously needs to be merged with trunk, and the `-fclang-abi-compat=` checks and release notes need to be updated to match the latest release version.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

erichkeane added a comment. In D147655#4251984 , @rsmith wrote: > In D147655#4251042 , @aaron.ballman > wrote: > >> In D147655#4250922 , @royjacobson >> wrote: >> >>>

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4251042 , @aaron.ballman wrote: > In D147655#4250922 , @royjacobson > wrote: > >> In D147655#4250056 , @rsmith wrote: >> >>> There

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

aaron.ballman added a comment. In D147655#4250922 , @royjacobson wrote: > In D147655#4250056 , @rsmith wrote: > >> There has not been any stable ABI from any compiler targeting the Itanium >> C++ ABI for

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

royjacobson added a comment. In D147655#4250056 , @rsmith wrote: > There has not been any stable ABI from any compiler targeting the Itanium C++ > ABI for constrained templates prior to this change. I don't think we need to > worry too much about

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4248800 , @royjacobson wrote: > I agree it doesn't affect too much code, but people do instantiate templates > manually sometimes. IIUC, linking against shared libraries would break if > that library does explicit

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

royjacobson added a comment. I agree it doesn't affect too much code, but people do instantiate templates manually sometimes. IIUC, linking against shared libraries would break if that library does explicit instantiations of constrained functions. That concerns me a bit. Also, do you know if

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

erichkeane accepted this revision. erichkeane added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:502 ArrayRef LocalParameters, + SourceLocation RParenLoc, ArrayRef Requirements, rsmith wrote: >

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:502 ArrayRef LocalParameters, + SourceLocation RParenLoc, ArrayRef Requirements, erichkeane wrote: > Is this an unrelated change? Are

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

erichkeane added a comment. I can't really review the libcxxabi parts, or the llvm-demangler parts, but everything looks right to me. I've got a pair of quick questions, otherwise I think this is going to be fine for me. Note the extra paren changes are something I think are valuable, but I'm

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. Here's the diff to `test_demangle.pass.cpp`: --- a/libcxxabi/test/test_demangle.pass.cpp +++ b/libcxxabi/test/test_demangle.pass.cpp @@ -29995,10 +29995,10 @@ const char* cases[][2] = {"_Z15test_uuidofTypeI10TestStructEvDTu8__uuidofT_EE", "void