aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:373-387
+ bool CkdOperation = false;
+ SourceManager &SM = S.getSourceManager();
+ if (BuiltinID == Builtin::BI__builtin_add_overflow &&
+ TheCall->getExprLoc().isMacroID() && Lexer::getImmediateMacroName(
+ TheCall->getExprLoc(), SM, S.getLangOpts()) == "ckd_add") {
+ CkdOperation = true;
+ } else if (BuiltinID == Builtin::BI__builtin_sub_overflow &&
----------------
How about something like this to avoid the code duplication?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:393-396
+ return (BT->getKind() >= BuiltinType::Short &&
+ BT->getKind() <= BuiltinType::Int128) || (
+ BT->getKind() >= BuiltinType::UShort &&
+ BT->getKind() <= BuiltinType::UInt128);
----------------
I think this should get us what we need.
I think `wchar_t` behavior is fine; in C++, that's a builtin datatype and
should not be used with these APIs and in C, that's a typedef to some integer
type and is allowed.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:407-408
QualType Ty = Arg.get()->getType();
- if (!Ty->isIntegerType()) {
+ bool Isvalid = CkdOperation ? ValidCkdIntType(Ty) : Ty->isIntegerType();
+ if (!Isvalid) {
S.Diag(Arg.get()->getBeginLoc(), diag::err_overflow_builtin_must_be_int)
----------------
Minor naming nit.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:388
+ }
+
// First two arguments should be integers.
----------------
ZijunZhao wrote:
> aaron.ballman wrote:
> >
> I keeps my original code because it just checks once but this checks many
> times and in case `BuiltinType::WChar_U` checking is missing(I know
> isWideCharType() can be added but I still think check the type is between
> Short and Int128, and UShort and UInt128 will be quicker and a bit safer?).
Yeah, I wasn't happy with how many extra checks my variant would come up with,
but I am presuming the optimizer is able to combine them into something
reasonable. I was more concerned about readability of the code, but I think we
can fix yours up and it's still sufficiently readable.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:414-431
if (!PtrTy ||
!PtrTy->getPointeeType()->isIntegerType() ||
+ (!PtrTy->getPointeeType()->isPureIntegerType() && CkdOperation) ||
PtrTy->getPointeeType().isConstQualified()) {
S.Diag(Arg.get()->getBeginLoc(),
diag::err_overflow_builtin_must_be_ptr_int)
<< Ty << Arg.get()->getSourceRange();
----------------
ZijunZhao wrote:
> aaron.ballman wrote:
> >
> I don't think the `else if` part should be removed. We should make sure the
> result type is not short type. In `ValidCkdIntType()` checking, short type is
> a valid type.
I think the `else if` should be removed; there's no reason the result type
cannot be `short`. e.g.,
```
short s1 = 1, s2 = 1, s3;
ckd_add(&s3, s1, s2);
```
Yes, `s1` and `s2` are promoted to `int`, but `1 + 1` can be represented in
`short` just fine.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157331/new/
https://reviews.llvm.org/D157331
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
