aaron.ballman added a subscriber: jrtc27.
aaron.ballman added inline comments.
================
Comment at: clang/docs/ReleaseNotes.rst:113-115
+- Clang now adds <stdckdint.h> which defines several macros for performing
+ checked integer arithmetic. The macro ``__STDC_VERSION_STDCKDINT_H__``
+ is an integer constant expression with a value equivalent to ``202311L``.
----------------
No need to mention the version macro in this case (I did it above because
`__STDC_VERSION__` is a commonly used and previously had a placeholder value
instead of a final value).
================
Comment at: clang/lib/Headers/stdckdint.h:12
+#define __STDCKDINT_H
+
+/* C23 7.20.1 Defines several macros for performing checked integer
arithmetic*/
----------------
Should a hosted build attempt to do an include_next into the system library and
then fall back to the compiler builtins, or should we treat this like stdbool.h
where the compiler always wins? CC @jyknight @jrtc27 @efriedma
My intuition is that we want to include_next in case the system has better
facilities than the compiler does.
================
Comment at: clang/lib/Headers/stdckdint.h:14
+/* C23 7.20.1 Defines several macros for performing checked integer
arithmetic*/
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))
----------------
You should also have a macro definition:
```
#define __STDC_VERSION_STDCKDINT_H__ 202311L
```
I've not yet added the `__STDC_VERSION_*` macros to the other headers we
provide because those require some further testing to determine if we conform
to C23 or not. However, because this is a brand new header being implemented,
we're doing that conformance testing now, so we should go ahead and implement
the version macro now.
================
Comment at: clang/lib/Headers/stdckdint.h:1
+/*===---- stdckdint.h - Standard header for checking integer
+ *-------------------------===
----------------
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).
================
Comment at: clang/lib/Lex/PPDirectives.cpp:234-237
+ .Cases("stdatomic.h", "stdbool.h", "stdckdint.h", "stddef.h", "stdint.h",
+ "stdio.h", true)
.Cases("stdlib.h", "stdnoreturn.h", "string.h", "tgmath.h", "threads.h",
true)
.Cases("time.h", "uchar.h", "wchar.h", "wctype.h", true)
----------------
I was thinking of something more along these lines.
================
Comment at: clang/test/C/C2x/n2359.c:40
+#error "__STDC_VERSION_STDCKDINT_H__ not defined"
+// expected-error@-1 {{"__STDC_VERSION_STDCKDINT_H__ not defined"}}
+#endif
----------------
ZijunZhao wrote:
> enh wrote:
> > don't you need another test somewhere that this _is_ defined under some
> > circumstances? (and a definition in the header itself!)
> I follow the cases like `__STDC_VERSION_LIMITS_H__` and
> `__STDC_VERSION_STDATOMIC_H__` . They are not defined in the <limits.h> or
> <stdatomic.h>.
I commented on that a bit above.
================
Comment at: clang/test/C/C2x/n2683.c:4
+/* WG14 N2683: Clang 18
+ * Define several macros for performing checked integer arithmetic
+ */
----------------
The first line of the comment is the paper number and whether we support it or
not, and the second line is the title of the paper, which is why there's less
information here now. :-)
================
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);
----------------
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.
================
Comment at: clang/test/C/C2x/n2683.c:19
+ bool flag_mul = ckd_mul(&result, day, char_var);
+}
+
----------------
Also, can you change the test file to use two spaces for indentation instead of
four? Same in the other test file.
================
Comment at: clang/test/C/C2x/n2683_2.c:1-2
+// RUN: %clang_cc1 -emit-llvm -o - -std=c23 %s | FileCheck %s
+// expected-no-diagnostics
+
----------------
This kind of comment is used when passing `-verify` on the RUN line and the
test has no diagnostics. Since we're not passing `-verify`, there's no need for
the comment.
================
Comment at: clang/test/Headers/stdckdint.c:1-2
+// RUN: %clang_cc1 -emit-llvm -verify -fgnuc-version=4.2.1 -std=c23 %s -o - |
FileCheck %s
+// expected-no-diagnostics
+#include <stdckdint.h>
----------------
I'm assuming that `-fgnuc-version=4.2.1` is not needed for the test 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
