rjmccall added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } ---------------- I know this is existing code, but this is a broken mess. Please change this to: ``` ExprResult Arg = DefaultFunctionArrayLvalueConversion(TheCall->getArg(2)); if (Arg.isInvalid()) return true; TheCall->setArg(2, Arg.get()); QualType Ty = Arg.get()->getType(); const auto *PtrTy = Ty->getAs<PointerType>(); if (!PtrTy || !PtrTy->getPointeeType()->isIntegerType() || PtrTy->getPointeeType().isConstQualified()) { S.Diag(Arg.get()->getBeginLoc(), diag::err_overflow_builtin_must_be_ptr_int) << Ty << Arg.get()->getSourceRange(); return true; } ``` Test case would be something like (in ObjC): ``` @interface HasPointer @property int *pointer; @end void test(HasPointer *hp) { __builtin_add_overflow(x, y, hp.pointer); } ``` And the earlier block needs essentially the same change (but obviously checking for a different type). You can't check types before doing placeholder conversions, and once you do l-value conversions and a type-check, you really don't need the full parameter-initialization logic. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:337 + QualType Ty = + I < 2 ? Arg.get()->getType() + : Arg.get()->getType()->getAs<PointerType>()->getPointeeType(); ---------------- jtmott-intel wrote: > erichkeane wrote: > > Try: > > > > Type *Ty = ArgExpr->getType()->getPointeeOrArrayElementType(); > > > > instead of the ternary. > A complication I'm seeing is that `getPointeeOrArrayElementType` returns a > `Type` but getIntWidth wants a `QualType`. Some options that occurred to me: > > - Is it acceptable to just wrap a `Type` in a `QualType`? > - I might be able to add a `getIntWidth(const Type*)` overload. > - Looks like I can skip the `getAs<PointerType>()` and just call > `getPointeeType` directly. The line would be shorter but I would still need > the ternary. * Is it acceptable to just wrap a Type in a QualType? In general, no, this can lose qualifiers on the array element. But `getIntWidth` shouldn't ever care about that, so in the abstract, adding the overload should be fine. However, in this case I think the ternary is actually better code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81420/new/ https://reviews.llvm.org/D81420 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits