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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
