[PATCH] D157331: [clang] Implement C23 <stdckdint.h>

[Aaron Ballman via Phabricator via cfe-commits]

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