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

Shafik Yaghmour via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Feb 24 15:46:45 PST 2023

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.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list