[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Andrey Ali Khan Bolshakov via Phabricator via lldb-commits]

bolshakov-a added inline comments.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+    if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+            TA.getUncommonValueType(), TA.getAsUncommonValue())) {
----------------
erichkeane wrote:
> bolshakov-a wrote:
> > erichkeane wrote:
> > > Has microsoft implemented this yet?  Can we see what they chose to do and make sure we match them? 
> > Experimentally, I've made me sure that MSVC produces the same mangled names for the cases provided in the `mangle-ms-templates` test as in the test. But there are problems with references to subobjects: some cases are explicitly unsupported in `mangleTemplateArgValue`, and some cases produce a magled name different from what MSVC does. Should it be fixed in this PR, or may be delayed?
> We need to end up doing our best to match the microsoft mangling if at all possible, since they own the ABI.  I DEFINITELY would want any followup patch to be promised for Clang17 (that is, we don't release Clang17 with this patch and NOT that patch), so I'd expect said patch to be available for review before this gets committed.
> 
> As far as whether it needs to happen in THIS patch, we can perhaps decide based on the severity of the break, if you can provide examples (or, if it is split into a separate patch, we can use the tests there).
I've addressed some issues already present on the main branch in [D146386](https://reviews.llvm.org/D146386). I could try to fix remaining issues in this PR afrer landing that one.


================
Comment at: clang/lib/AST/TemplateBase.cpp:244
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+    // FIXME: The Declaration form should expose a const ValueDecl*.
+    initFromDeclaration(const_cast<ValueDecl *>(VD), Type, IsDefaulted);
----------------
erichkeane wrote:
> Why can this not happen now?
Adding `const` to the `TemplateArgument::DA::D` type and to the `TemplateArgument::getAsDecl()` return type would lead to many changes unrelated to this PR.


================
Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+    // FIXME: Visit value.
+    break;
----------------
bnbarham wrote:
> bolshakov-a wrote:
> > bnbarham wrote:
> > > bolshakov-a wrote:
> > > > bnbarham wrote:
> > > > > akyrtzi wrote:
> > > > > > erichkeane wrote:
> > > > > > > bolshakov-a wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Any particular reason this isn't being handled now?
> > > > > > > > I need some guidance here. Which characters are allowed in the USR? Could the mangling algorithm from `CXXNameMangler::mangleValueInTemplateArg` be moved into some common place and reused here?
> > > > > > > I have no idea what is valid here.  BUT @akyrtzi and @gribozavr (or @gribozavr2 ?) seem to be the ones that touch these files the most?
> > > > > > Adding @bnbarham to review the `Index` changes.
> > > > > Just visiting the underlying type seems reasonable, ie. `VisitType(Arg.getUncommonValueType());`. If it needed to be differentiated between a `TemplateArgument::Type` you could add a prefix character (eg. `U`), but that doesn't seem needed to me.
> > > > Doesn't the holded value be added so as to distinguish e.g. `Tpl<1.5>` from `Tpl<2.5>`?
> > > Ah I see, yeah, we would. And there's no USR generation for APValue currently, which I assume is why your original question came up.
> > > 
> > > In general a USR just wants to uniquely identify an entity across compilations and isn't as restricted as the mangled name. For basically everything but `LValue` it seems like you'd be fine to print the value (for eg. int, float, etc), visit the underlying type (array, vector), or the visit the underlying decl (struct, union, member pointer). That's almost true for `LValue` as well, just with the extra parts that are also added to the ODR hash.
> > > 
> > > Alternatively, you could also just print the hash from `Profile` with the same handling as ODR hash. Worst case we'd accidentally merge specializations, but if that's good enough for the ODR hash it's probably good enough here as well.
> > > it seems like you'd be fine to print the value (for eg. int, float, etc)
> > 
> > I'm in doubt about the dot inside a floating point value representation. Minus sign is allowed, as I can see for `TemplateArgument::Integral` case.
> As long as there's a prefix for APValue and its kind, the dot is fine (eg. maybe `@AP@` and then `f` for float, `i` for integer, etc).
Thank you! I've decided to go the simplest way, i. e. to use `ODRHash` here. Should I write a test case (or some test cases), or they could become fragile due to possible `ODRHash` implementation changes? I've checked USR locally a little.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140996/new/

https://reviews.llvm.org/D140996