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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I have some suggested changes for the way we're checking and diagnosing unsuitable types; I've not tried my suggestions out though, so if you run into problems, please let me know.



================
Comment at: clang/include/clang/AST/Type.h:2159
   bool isEnumeralType() const;
+  bool isPureIntegerType() const;
 
----------------
I'd rather put this logic into SemaChecking.cpp than expose it in Type.h. This isn't a type predicate most other code should need to use.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8616-8625
 def err_overflow_builtin_must_be_int : Error<
   "operand argument to overflow builtin must be an integer (%0 invalid)">;
 def err_overflow_builtin_must_be_ptr_int : Error<
   "result argument to overflow builtin must be a pointer "
   "to a non-const integer (%0 invalid)">;
 def err_overflow_builtin_bit_int_max_size : Error<
   "__builtin_mul_overflow does not support 'signed _BitInt' operands of more "
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8624-8625
   "than %0 bits">;
+def warn_overflow_builtin_can_not_be_short : Warning<
+  "'short' may not be suitable to hold the result of operating two 'int's ">;
 def err_expected_struct_pointer_argument : Error<
----------------
There's a few things that aren't correct here, so I'm suggesting a different approach, but for your own understanding:

* All warning diagnostics need to be added to a warning group, using `InGroup<>`.
* The warning text is very specific to only one problematic situation; it should be generalized to cover as many code patterns as it can.
* We can likely reuse the existing error (`err_overflow_builtin_must_be_int`) instead of making a new one.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:388
+  }
+
   // First two arguments should be integers.
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:396-401
+    bool IsInt = CkdOperation ? Ty->isPureIntegerType() : Ty->isIntegerType();
+    if (!IsInt) {
       S.Diag(Arg.get()->getBeginLoc(), diag::err_overflow_builtin_must_be_int)
           << Ty << Arg.get()->getSourceRange();
       return true;
     }
----------------



================
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();
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331