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

Fri Nov 3 07:20:42 PDT 2023

bolshakov-a added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2205-2206
   "bit-field%select{| %1}2">;
+def err_reference_bind_to_bitfield_in_cce : Error<
+  "reference cannot bind to bit-field in converted constant expression">;
 def err_reference_bind_to_vector_element : Error<
----------------
aaron.ballman wrote:
> This change seems somewhat orthogonal to the rest of the patch; should this be split out? Also, there doesn't appear to be test coverage for the new diagnostic.
Separated into https://github.com/llvm/llvm-project/pull/71077

The problem showed up after one of rebasings when C++20 mode was turned on for `CXX/drs/dr12xx.cpp` test in [that commit](https://github.com/llvm/llvm-project/commit/653a82e95257a7cd3f22c24e40d54459a6608429).

================
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:
> bolshakov-a wrote:
> > bolshakov-a wrote:
> > > efriedma wrote:
> > > > bolshakov-a wrote:
> > > > > erichkeane wrote:
> > > > > > erichkeane wrote:
> > > > > > > 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?
> > > > > > This is still an open, and we need @rjmccall @eli.friedman or @asl to help out here.
> > > > > Ping @efriedma, @rjmccall, @asl.
> > > > I'm not really familiar with the mangling implications for this particular construct, nor am I actively involved with the Itanium ABI specification, so I'm not sure how I can help you directly.
> > > > 
> > > > That said, as a general opinion, I don't think it's worth waiting for updates to the Itanuim ABI  document to be merged; such updates are happening slowly at the moment, and having a consistent mangling is clearly an improvement even if it's not specified.  My suggested plan of action:
> > > > 
> > > > - Make sure you're satisfied the proposed mangling doesn't have any holes you're concerned about (i.e. it produces a unique mangling for all the relevant cases).  If you're not sure, I can try to spend some time understanding this, but it doesn't sound like you have any concerns about this.
> > > > - Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang.  Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
> > > > - Go ahead with this.
> > > > Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
> > > 
> > > I'm sorry for noting one more time that Richard already pushed these changes in clang upstream, but they had been just reverted.
> > > 
> > > Maybe, I should make a PR into Itanium API repository, but I probably need some time to dig into the theory and all the discussions. But yes, even NTTP argument mangling rules are not still merged: https://github.com/itanium-cxx-abi/cxx-abi/pull/140
> > @aaron.ballman, @erichkeane, seems like it is already fixed in the ABI document:
> > > Typically, only references to function template parameters occurring within the dependent signature of the template are mangled this way. In other contexts, template instantiation replaces references to template parameters with the actual template arguments, and mangling should mangle such references exactly as if they were that template argument.
> > 
> > https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.template-param
> > 
> > See also [the discussion in the issue](https://github.com/itanium-cxx-abi/cxx-abi/issues/111#issuecomment-1567486892).
> Okay, I think I agree that this is already addressed in the ABI document. I think we can drop the comment referencing the ABI issue, wdyt?
Ok, dropped.

================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7968
+  case APValue::FixedPoint:
+    return FixedPointLiteral::CreateFromRawInt(
+        S.Context, Val.getFixedPoint().getValue(), T, Loc,
----------------
aaron.ballman wrote:
> What should happen if `T` isn't a fixed point type? (Should we assert here?)
I don't expect that it will happen. Non-type template argument is a constant expression converted to the type of the template parameter. Hence, a value produced by such an expression should correspond to the NTTP type.

Should an assert be placed here, it should be placed in the other `switch` branches as well, I think. The problem is with `BuildExpressionFromNonTypeTemplateArgumentValue` interface: the parameters `T` and `Val` of this function aren't fully independent from each other (likewise `Type` and `Value` fields of the `TemplateArgument::V` structure). I'm not sure whether it is worth to be fixed here somehow.

================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8033
+
+  case TemplateArgument::NullPtr:
+  case TemplateArgument::Declaration:
----------------
aaron.ballman wrote:
> I'm a bit surprised that nullptr is modeled as a declaration rather than an integral value; can you explain that a bit? (I do see that the existing code in `BuildExpressionFromDeclTemplateArgument()` does have a case for handling nullptr, so the changes may be fine as-is.)
> 
> I don't test coverage for use of `nullptr_t` as an NTTP, unless I've missed it.
`TemplateArgument::NullPtr` represents usually a `nullptr` given for a template parameter of pointer-to-object or pointer-to-member type, i.e. a parameter referring in other cases to a declaration of some object with static storage duration. For this reason, it can be considered as a special case of declaration-referring template argument. `nullptr` can also be an argument for NTTP of `std::nullptr_t` type, but I have no idea when it is worth to use such a parameter in real code. (Again, this change is from [the original Richard's commit](https://github.com/llvm/llvm-project/commit/4b574008aef5a7235c1f894ab065fe300d26e786). I'm just guessing what he had in mind.)

`temp_arg_nontype_cxx11.cpp`, `temp_arg_nontype_cxx1z.cpp`, and `CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp` have already some cases of using null pointers as template arguments, e.g. [here](https://github.com/llvm/llvm-project/blob/llvmorg-18-init/clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp#L25) and [here](https://github.com/llvm/llvm-project/blob/llvmorg-18-init/clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp#L59).

================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:6303-6305
   case TemplateArgument::NullPtr:
-    MarkUsedTemplateParameters(Ctx, TemplateArg.getNullPtrType(), OnlyDeduced,
-                               Depth, Used);
----------------
aaron.ballman wrote:
> Can you explain this change a bit?
I think the idea is that handling `TemplateArgument::NullPtr` case here is just useless. This function is used in the process of determination which template parameters of a templated function, deduction guide, or partial template specialization could be deduced. Such parameters are searched in parameter-declaration-clause, or in the template-argument-list of the simple-template-id of a partial template specialization. But when a NTTP is used as an argument of another template, that argument has the `TemplateArgument::Expression` kind and stores an expression referring to the NTTP. E.g., given such a template and its partial specialization:
```
template <int K, int N>
struct Templated {};
template <int M>
struct Templated<1, M> {};
```
the argument `1` has the `TemplateArgument::Integral` but is irrelevant to the `M` value deduction, whereas the subsequent argument `M` has the `TemplateArgument::Expression` kind. The `nullptr` case is similar.

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

https://reviews.llvm.org/D140996