[PATCH] D81420: Fix size for _ExtInt types with builtins

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:327
       return true;
     TheCall->setArg(2, Arg.get());
   }
----------------
rjmccall wrote:
> 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.
I've implemented this locally but I have a quick question about the test. It passed even before I applied this code change. Is this test expected to fail before this change and pass after? Or is it just to prove that the feature (placeholder argument types?) works in general?


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

https://reviews.llvm.org/D81420