efriedma added inline comments.
================ Comment at: clang/lib/AST/ItaniumMangle.cpp:4579 + if (CE->hasAPValueResult()) + mangleValueInTemplateArg(ParamType, CE->getResultAsAPValue(), false, + /*NeedExactType=*/true); ---------------- I'm not sure what the point of the `if (CE->hasAPValueResult())` is; are you just trying to avoid copying the APValue? (If this is going to be a repeating pattern, maybe we can add some sort of utility class to represent the pattern.) ================ Comment at: clang/lib/AST/ItaniumMangle.cpp:4397 + // argument. + // As proposed in https://github.com/itanium-cxx-abi/cxx-abi/issues/111. + auto *SNTTPE = cast<SubstNonTypeTemplateParmExpr>(E); ---------------- bolshakov-a wrote: > erichkeane wrote: > > erichkeane wrote: > > > aaron.ballman wrote: > > > > We should get this nailed down. It was proposed in Nov 2020 and the > > > > issue is still open. CC @rjmccall > > > This definitely needs to happen. @rjmccall or @eli.friedman ^^ Any idea > > > what the actual mangling should be? > > This is still an open, and we need @rjmccall @eli.friedman or @asl to help > > out here. > Ping @efriedma, @rjmccall, @asl. I'm not really familiar with the mangling implications for this particular construct, nor am I actively involved with the Itanium ABI specification, so I'm not sure how I can help you directly. That said, as a general opinion, I don't think it's worth waiting for updates to the Itanuim ABI document to be merged; such updates are happening slowly at the moment, and having a consistent mangling is clearly an improvement even if it's not specified. My suggested plan of action: - Make sure you're satisfied the proposed mangling doesn't have any holes you're concerned about (i.e. it produces a unique mangling for all the relevant cases). If you're not sure, I can try to spend some time understanding this, but it doesn't sound like you have any concerns about this. - Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications. - Go ahead with this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140996/new/ https://reviews.llvm.org/D140996 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
