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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


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


================
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*.
----------------
erichkeane wrote:
> 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.
I am going to third this sentiment, this is definitely not the right approach and the fact that you have this ad-hoc case below here also speaks to this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996