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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Headers/stdckdint.h:13
+
+#define __STDC_VERSION_STDCKDINT_H__ 202311L
+
----------------
I think this should only be defined when the function-like macros are being defined, so this should move down into the `#if` block below. That way, people can do:
```
#if __STDC_VERSION_STDCKDINT_H__  >= 202311L
```
to know they've got access to the macros. Which is weird because they could also do `#ifdef ckd_add`, but oh well, C is weird.


================
Comment at: clang/lib/Headers/stdckdint.h:1
+/*===---- stdckdint.h - Standard header for checking integer
+ *-------------------------===
----------------
ZijunZhao wrote:
> aaron.ballman wrote:
> > hiraditya wrote:
> > > nit: format.
> > The formatting for this is still off (it line wraps here and on line 7-8 -- it should be shortened so that the closing comment fits within the 80 col limit).
> oh yes I set 80 col limit and they don't exceed the line. {F28780476} 
> I attach the screenshort and hope it help explain. I think I might misunderstand something?
Oh! I was thrown off by the C-style commenting, but you're right, this is correct as-is (and matches how the other C headers look), sorry for the noise!


================
Comment at: clang/test/C/C2x/n2683.c:16
+
+    bool flag_add = ckd_add(&result, a33, char_var);
+    bool flag_sub = ckd_sub(&result, bool_var, day);
----------------
ZijunZhao wrote:
> aaron.ballman wrote:
> > It looks like the builtins are missing some checks that are required by the C standard.
> > 
> > 7.20p3: Both type2 and type3 shall be any integer type other than "plain" char, bool, a bit-precise integer type, or an enumerated type, and they need not be the same.  ...
> > 
> > So we should get a (warning?) diagnostic on all of these uses.
> > 
> > We should also add a test when the result type is not suitable for the given operand types. e.g.,
> > ```
> > void func(int one, int two) {
> >   short result;
> >   ckd_add(&result, one, two); // `short` may not be suitable to hold the result of adding two `int`s
> > 
> >   const int other_result = 0;
> >   ckd_add(&other_result, one, two); // `const int` is definitely not suitable because it's not a modifiable lvalue
> > }
> > ```
> > This is because of:
> > 
> > 7.20.1p4: It is recommended to produce a diagnostic message if type2 or type3 are not suitable integer types, or if *result is not a modifiable lvalue of a suitable integer type.
> yes, I am trying to add the tests about `short` type and `const` variable but there is no warning about inappropriate type 😢 and for const one I get ```result argument to overflow builtin must be a pointer to a non-const integer ('const int *' invalid)``` error 😂 
I think the diagnostic with a `const` result is reasonable enough (unfortunate it talks about builtins but we can correct the wording later). The other type checking is something that will need to be added to: https://github.com/llvm/llvm-project/blob/8f6a1a07cb85980013c70d5af6d28f5fcf75e732/clang/lib/Sema/SemaChecking.cpp#L368

One thing you'll have to do is determine whether the builtin is being used via the standard interface or whether it's being used directly by the user. We don't want to change the behavior of the call through `__builtin_add_overflow` because that can break existing code. So you'll need to see if the call expression is a macro expansion from a macro named `ckd_add` (for example) and perform the extra checking only in that case.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331