bnbarham added inline comments.
================ Comment at: clang/lib/Index/USRGeneration.cpp:1032 + case TemplateArgument::UncommonValue: + // FIXME: Visit value. + break; ---------------- bolshakov-a wrote: > 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. You could add a test that checks the ref has the same USR as the def, but yeah, I wouldn't specifically check the USR here. 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
