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

Mon Mar 13 15:09:06 PDT 2023

bnbarham added inline comments.

================
Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+    // FIXME: Visit value.
+    break;
----------------
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.

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

https://reviews.llvm.org/D140996