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