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
