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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
