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

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.
Herald added a subscriber: wangpc.

I've not spotted any major concerns with this patch, but I did have some minor nits to look into. I'd love to hear from @rsmith and @erichkeane before signing off on this, as the changes are pretty involved and they've both done some in-depth looks at the patch already.



================
Comment at: clang/include/clang/AST/TemplateBase.h:168-173
+  void initFromType(QualType, bool isNullPtr, bool IsDefaulted);
+  void initFromDeclaration(ValueDecl *, QualType, bool IsDefaulted);
+  void initFromIntegral(const ASTContext &, const llvm::APSInt &, QualType,
+                        bool IsDefaulted);
+  void initFromStructural(const ASTContext &, QualType, const APValue &,
+                          bool IsDefaulted);
----------------
Can you add parameter names for the unnamed ones (we typically only leave a parameter unnamed when it is unused).


================
Comment at: clang/include/clang/AST/TemplateBase.h:382-385
+  /// Get the value of an StructuralValue.
+  const APValue &getAsStructuralValue() const { return *Value.Value; }
+
+  /// Get the type of an StructuralValue.
----------------



================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1624-1626
+  return const_cast<ValueDecl *>(
+      V.getLValueBase().dyn_cast<const ValueDecl *>());
+}
----------------
Does this work or is the `const_cast` actually required?


================
Comment at: clang/lib/AST/ODRHash.cpp:1324-1327
+        if (auto *AT = TypeSoFar->getAsArrayTypeUnsafe()) {
+          if (auto *CAT = dyn_cast<ConstantArrayType>(AT))
+            OnePastTheEnd |= CAT->getSize() == E.getAsArrayIndex();
+          TypeSoFar = AT->getElementType();
----------------



================
Comment at: clang/lib/AST/ODRHash.cpp:1330
+          const Decl *D = E.getAsBaseOrMember().getPointer();
+          if (auto *FD = dyn_cast<FieldDecl>(D)) {
+            if (FD->getParent()->isUnion())
----------------



================
Comment at: clang/lib/AST/ODRHash.cpp:1341-1342
+    }
+    ID.AddInteger((Value.isNullPointer() ? 1 : 0) | (OnePastTheEnd ? 2 : 0) |
+                  (Value.hasLValuePath() ? 4 : 0));
+    break;
----------------
I think this might be a bit clearer, but I don't insist on the change.


================
Comment at: clang/lib/AST/TemplateBase.cpp:163
 
-TemplateArgument::TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value,
-                                   QualType Type, bool IsDefaulted) {
+void TemplateArgument::initFromType(QualType T, bool isNullPtr,
+                                    bool IsDefaulted) {
----------------



================
Comment at: clang/lib/AST/TemplateBase.cpp:408-409
   case Integral:
-    getAsIntegral().Profile(ID);
     getIntegralType().Profile(ID);
+    getAsIntegral().Profile(ID);
+    break;
----------------
Why did the order of these calls change?


================
Comment at: clang/lib/AST/TemplateBase.cpp:619
+  case TemplateArgument::UncommonValue: {
+    // FIXME: We're guessing at LangOptions!
+    SmallString<32> Str;
----------------
bolshakov-a wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > It's probably okay enough, but this is now the third instance of adding the same bug to this helper method -- maybe we should fix that instead?
> > Agreed, seems to me we should do the work NOW to just wire in the lang-opts to this function.
> The problem here is because this function is called from a stream insertion operator, and there isn't obviously any way to pass 3rd parameter into it without switching it into an ordinary function.
Okay, that's reasonable enough to hold off on changing. Thanks!


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


================
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>;
 
----------------
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.


================
Comment at: clang/www/cxx_status.html:1061
+        <details>
+          <summary>Clang 17 (Partial)</summary>
+          Reference type template arguments referring to instantiation-dependent objects and subobjects
----------------



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

https://reviews.llvm.org/D140996