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

[Andrey Ali Khan Bolshakov via Phabricator via cfe-commits]

bolshakov-a added a comment.

Btw, formatting of unrelated lines has leaked into `TemplateBase.h`. Sorry.



================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1624-1626
+  return const_cast<ValueDecl *>(
+      V.getLValueBase().dyn_cast<const ValueDecl *>());
+}
----------------
aaron.ballman wrote:
> Does this work or is the `const_cast` actually required?
No, it doesn't compile, likewise the standard C++ `dynamic_cast` cannot remove constness.


================
Comment at: clang/lib/AST/TemplateBase.cpp:408-409
   case Integral:
-    getAsIntegral().Profile(ID);
     getIntegralType().Profile(ID);
+    getAsIntegral().Profile(ID);
+    break;
----------------
aaron.ballman wrote:
> Why did the order of these calls change?
I don't know, it is from 9e08e51a20d0d2. I've tried to invert the order along with the order for `StructuralValue`, and all tests have been passed.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:5983-5985
+      Expr *E = Result.get();
+      if (!isa<ConstantExpr>(E))
+        E = ConstantExpr::Create(S.Context, Result.get(), Value);
----------------
aaron.ballman wrote:
> I thought we could run into situations where we had a `ConstantExpr` but it did not yet have its result stored in it. Should we assert that isn't the case here?
If I understand correctly, the sole place where `ConstantExpr` is constructed which may occur here is `BuildExpressionFromNonTypeTemplateArgumentValue` function, and a value is set into it there. Should I add the assertion into code?


================
Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:56
+using CF = ComplexFloat<1.0f + 3.0fi>;
+using CF = ComplexFloat<3.0fi + 1.0f>;
 
----------------
aaron.ballman wrote:
> bolshakov-a wrote:
> > shafik wrote:
> > > Can we add an enum example e.g.:
> > > 
> > > ```
> > > enum E{ E1, E2};
> > > template <E> struct SE {};
> > > using IPE = SE<E1>;
> > > using IPE = SE<E2>;
> > > 
> > > ```
> > What for? Enumerators as non-type template arguments are allowed since C++98, AFAIK. And this test is about changes in C++20.
> Sometimes we're lacking coverage for existing features, so when updating code in the area, we'll sometimes ask for extra coverage just to be sure we're not regressing something we think might not have a lot of existing test coverage.
`temp_arg_nontype.cpp` test already has some `enum` cases. If a case with type alias should be added, it shoud be added there, not in the `temp_arg_nontype_cxx20.cpp`, I think.


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

https://reviews.llvm.org/D140996