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

[Erich Keane via Phabricator via lldb-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:3036
     if (!std::is_trivially_destructible<T>::value) {
-      auto DestroyPtr = [](void *V) { static_cast<T *>(V)->~T(); };
-      AddDeallocation(DestroyPtr, Ptr);
+      auto DestroyPtr = [](void *V) { ((T *)V)->~T(); };
+      AddDeallocation(DestroyPtr, (void *)Ptr);
----------------
This change is weird... what is going on here?


================
Comment at: clang/include/clang/AST/TemplateBase.h:88
+    /// so cannot be dependent.
+    UncommonValue,
+
----------------
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. 


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+    if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+            TA.getUncommonValueType(), TA.getAsUncommonValue())) {
----------------
Has microsoft implemented this yet?  Can we see what they chose to do and make sure we match them? 


================
Comment at: clang/lib/AST/ODRHash.cpp:177
+    case TemplateArgument::UncommonValue:
+      // FIXME: Include a representation of these arguments.
       break;
----------------
I feel like this DEFINITELY needs to happen.  Nullptr/integral is less important, but now that we have an actual value here, I think it needs to be part of the hash.


================
Comment at: clang/lib/AST/TemplateBase.cpp:204-211
+  if (Type->isIntegralOrEnumerationType() && V.isInt())
+    *this = TemplateArgument(Ctx, V.getInt(), Type);
+  else if ((V.isLValue() && V.isNullPointer()) ||
+           (V.isMemberPointer() && !V.getMemberPointerDecl()))
+    *this = TemplateArgument(Type, /*isNullPtr=*/true);
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+    // FIXME: The Declaration form should expose a const ValueDecl*.
----------------
aaron.ballman wrote:
> Well this is certainly a unique approach...  Personally, I think it'd be nice to not assign to `this` within a constructor by calling other constructor; that falls squarely into "wait, what, can you even DO that?" kind of questions for me.
I agree, this function needs to be refactored to call some sort of 'init' function or something, even if we have to refactor the other constructors.  This assignment to `*this` is just too strange to let stay.


================
Comment at: clang/lib/AST/TemplateBase.cpp:619
+  case TemplateArgument::UncommonValue: {
+    // FIXME: We're guessing at LangOptions!
+    SmallString<32> Str;
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7882
+    Sema &S, QualType T, const APValue &Val, SourceLocation Loc) {
+  auto MakeInitList = [&](ArrayRef<Expr *> Elts) -> Expr * {
+    auto *ILE = new (S.Context) InitListExpr(S.Context, Loc, Elts, Loc);
----------------
I don't have a good idea of what is happening in this function here, it isn't really clear... before committing, someone needs to do a deep dive on this function for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996