rjmccall added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:327
return true;
TheCall->setArg(2, Arg.get());
}
----------------
jtmott-intel wrote:
> 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?
Oh, we seem to handle placeholders specially, nevermind. Well, what I said
about doing conversions before checking the type still holds, but the test case
is just this:
```
int z[1];
__builtin_add_overflow(x, y, z);
```
where we should decay the argument to a pointer instead of rejecting it.
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
