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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
