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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added subscribers: eli.friedman, gribozavr2, akyrtzi, gribozavr.
erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/TemplateBase.h:88
+    /// so cannot be dependent.
+    UncommonValue,
+
----------------
bolshakov-a wrote:
> shafik wrote:
> > erichkeane wrote:
> > > I definitely hate the name here... Just `Value` makes a bit more sense, but isn't perfectly accurate.  Perhaps `NonTypeValue`?  But that is a little redundant.  `Uncommon` here is just strange and not particularly descriptive. 
> > This catch all `UncommonValue` feels artificial. Maybe we need a separate out the cases into more granular cases like `Float` etc....
> @erichkeane, it looks strange, I agree. Even just `CommonValue` sounds better for me (but my English is far from fluent). Maybe, `ArbitraryValue`?
> 
> @shafik, your suggestion would move this PR far enough from the original Richard's commit. And I'd prefer to merge `Declaration`, `Integral`, and `NullPtr` kinds into that is currently called `UncommonValue` rather than to repeat here various `APValue` kinds.
I don't think splitting out the individual cases has all that much value, at least until we NEED it.  

As far as a name, what about `StructuralValue`?  P1907 calls the 'type' of that A 'structural type'? It isn't perfect, but it at least seems to be somewhat defensible with standards language.


================
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);
----------------
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?


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+    if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+            TA.getUncommonValueType(), TA.getAsUncommonValue())) {
----------------
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).


================
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);
----------------
Why can this not happen now?


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


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7944
+  case APValue::Indeterminate:
+    // FIXME: Are these values possible?
+  case APValue::LValue:
----------------
Rather than this, I'd prefer these all down to the llvm_unreachable below.  If we find they are reachable, then we'd be expected to implement them at that point.


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

https://reviews.llvm.org/D140996