enh added inline comments.
================ Comment at: clang/lib/Headers/stdckdint.h:13 + +#if defined(__GNUC__) +#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R)) ---------------- hiraditya wrote: > xbolva00 wrote: > > yabinc wrote: > > > enh wrote: > > > > enh wrote: > > > > > enh wrote: > > > > > > ZijunZhao wrote: > > > > > > > enh wrote: > > > > > > > > is this ever _not_ set for clang? > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23 > > > > > > > I think it is set? > > > > > > i get an error from > > > > > > ``` > > > > > > /tmp$ cat x.c > > > > > > #if defined(__GNUC__) > > > > > > #error foo > > > > > > #endif > > > > > > ``` > > > > > > regardless of whether i compile with -std=c11 or -std=gnu11. > > > > > > neither -ansi nor -pedantic seem to stop it either. > > > > > it does look like it _should_ be possible to not have it set though? > > > > > llvm/llvm-project/clang/lib/Frontend/InitPreprocessor.cpp has: > > > > > ``` > > > > > if (LangOpts.GNUCVersion != 0) { > > > > > // Major, minor, patch, are given two decimal places each, so > > > > > 4.2.1 becomes > > > > > // 40201. > > > > > unsigned GNUCMajor = LangOpts.GNUCVersion / 100 / 100; > > > > > unsigned GNUCMinor = LangOpts.GNUCVersion / 100 % 100; > > > > > unsigned GNUCPatch = LangOpts.GNUCVersion % 100; > > > > > Builder.defineMacro("__GNUC__", Twine(GNUCMajor)); > > > > > Builder.defineMacro("__GNUC_MINOR__", Twine(GNUCMinor)); > > > > > Builder.defineMacro("__GNUC_PATCHLEVEL__", Twine(GNUCPatch)); > > > > > Builder.defineMacro("__GXX_ABI_VERSION", "1002"); > > > > > > > > > > if (LangOpts.CPlusPlus) { > > > > > Builder.defineMacro("__GNUG__", Twine(GNUCMajor)); > > > > > Builder.defineMacro("__GXX_WEAK__"); > > > > > } > > > > > } > > > > > ``` > > > > /me wonders whether the right test here is actually `#if > > > > __has_feature(__builtin_add_overflow)` (etc)... > > > > > > > > but at this point, you definitely need an llvm person :-) > > > From > > > https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins, > > > we can check them with > > > __has_builtin(__builtin_add_overflow) && > > > __has_builtin(__builtin_sub_overflow) && > > > __has_builtin(__builtin_mul_overflow). > > > I saw some code also checks if __GNUC__ >= 5: > > > > > > // The __GNUC__ checks can not be removed until we depend on GCC >= 10.1 > > > // which is the first version that returns true for > > > __has_builtin(__builtin_add_overflow) > > > #if __GNUC__ >= 5 || __has_builtin(__builtin_add_overflow) > > > > > > I guess we don't need to support real gcc using this header here. So > > > maybe only checking __has_builtin is enough? > > > > > > By the way, if __builtin_add_overflow may not appear on some targets, do > > > we need to modify tests to specify triple like "-triple > > > "x86_64-unknown-unknown"" in > > > https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/builtins-overflow.c#L5 > > > ? > > > > > #ifndef __has_builtin // Optional of course. > > #define __has_builtin(x) 0 // Compatibility with non-clang compilers. > > #endif > > > > ... > > #if __has_builtin(__builtin_trap) > > __builtin_trap(); > > #else > > abort(); > > #endif > > /me wonders whether the right test here is actually #if > > __has_feature(__builtin_add_overflow) (etc)... > > i think that should be added. > > I guess we also need a with `__STDC_VERSION__ > 202000L`? in princple we'd > have a C23 number for it but i'm not sure if that has been added to clang yet. > i think that should be added. i was advising the opposite --- now this is a standard C23 feature, any architectures where __builtin_*_overflow doesn't work need to be found and fixed. and we'll do that quicker if we unconditionally expose these and (more importantly!) run the tests. > I guess we also need a with __STDC_VERSION__ > 202000L? _personally_ i think that's silly because you can't hide the header file, so it doesn't make any sense to just have it empty if someone's actually #included it. we don't do this anywhere in bionic for example, for this reason. but obviously that's an llvm decision, and it does look like the other similar headers _do_ have this check, so, yeah, probably. ================ Comment at: clang/test/Headers/stdckdint.cpp:1 +// RUN: %clang_cc1 -emit-llvm -fgnuc-version=4.2.1 -std=gnu++11 %s -o - | FileCheck %s + ---------------- hiraditya wrote: > seems like we don't have a -std=gnu23, or -std=c23 standard flag for this in > clang yet. > > https://godbolt.org/z/7dKnGEWWE > > we probably need it before testing stdckdint i guess? other headers just use > and the previous version. (though see stdalign.h if you're looking for some random cleanup to do!) 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