Avoid warnings when compiling under `Wpedantic` #437

andrjohns · 2024-06-23T14:59:12Z

This PR suppresses warnings under Wpedantic compilation by explicitly marking GNU extensions with the __extension__ keyword and replacing the use of GNU zero-length arrays with ISO C flexible arrays.

Let me know if I've missed anything or should approach this differently, thanks!

For context I maintain an R interface for quickjs, and the R package manager (CRAN) requires all C/C++ code to compile under -Wpedantic without warnings. I currently have to patch the code for each release, so it would be great if that wasn't necessary.

But as you can see here the patchset is fairly minor, so I completely understand if you'd rather keep the GNU-isms as-is!

chqrlie

The only required change is the removal of the forward enum declaration.

chqrlie · 2024-06-23T15:17:21Z

libbf.c

+/* uint128_t defined in libbf.h          */
 #define muldq(r1, r0, a, b)                     \
    do {                                        \
-        unsigned __int128 __t;                          \
-        __t = (unsigned __int128)(a) * (unsigned __int128)(b);  \
+        uint128_t __t;                          \
+        __t = (uint128_t)(a) * (uint128_t)(b);  \
        r0 = __t;                               \
        r1 = __t >> 64;                         \
    } while (0)

 #define divdq(q, r, a1, a0, b)                  \
    do {                                        \
-        unsigned __int128 __t;                  \
+        uint128_t __t;                  \
        limb_t __b = (b);                       \
-        __t = ((unsigned __int128)(a1) << 64) | (a0);   \
+        __t = ((uint128_t)(a1) << 64) | (a0);   \


This was definitely an oversight. Should have used uint128_t from day one.

chqrlie · 2024-06-23T15:20:16Z

libbf.h

-typedef __int128 int128_t;
-typedef unsigned __int128 uint128_t;
+__extension__ typedef __int128 int128_t;
+__extension__ typedef unsigned __int128 uint128_t;
 typedef int64_t slimb_t;
 typedef uint64_t limb_t;
 typedef uint128_t dlimb_t;


Change OK as long as __extension__ is properly defined elsewhere if required.

We should probably not define int128_t and uint128_t if they do not exist and use bf_ prefixed names for all public types. Big Floats are expected to be replaced with a simpler and faster BigNum package anyway so fixing this is not necessary.

Change OK as long as extension is properly defined elsewhere if required.

Great! Just to clarify though, __extension__ is a standard compiler keyword, is there a place in the doc or similar where I should define it?

We should probably not define int128_t and uint128_t if they do not exist and use bf_ prefixed names for all public types.

According to GCC, __int128 should exist if long long is at least 128 bits wide. So these definitions and their usage in libbf.c could be guarded by a sizeof(long long) >= 16 conditional or similar. Is that something I should do here or save for a separate PR?

We should probably not define int128_t and uint128_t if they do not exist and use bf_ prefixed names for all public types.

According to GCC, __int128 should exist if long long is at least 128 bits wide. So these definitions and their usage in libbf.c could be guarded by a sizeof(long long) >= 16 conditional or similar. Is that something I should do here or save for a separate PR?

int128_t is potentially defined in <stdint.h>, I think you should use #ifndef INT128_MAX as a guard to avoid redefining int128_t if it exists already. sizeof cannot be used in preprocessor expressions.

Change OK as long as extension is properly defined elsewhere if required.

Great! Just to clarify though, __extension__ is a standard compiler keyword, is there a place in the doc or similar where I should define it?

__extension__ is not a standard keyword, it is a reserved name. This name is used by gcc, clang and other compilers, but not msvc. I was unsure when it was introduced in gcc and clang. Testing with godbolt.org, I checked that it is supported since at least gcc 3.4.3 and clang 3.0, so no problem with that.

extension is not a standard keyword, it is a reserved name. This name is used by gcc, clang and other compilers, but not msvc.

Good to know, I had no idea!

int128_t is potentially defined in <stdint.h>, I think you should use #ifndef INT128_MAX as a guard to avoid redefining int128_t if it exists already

Do any implementations provide int128_t? I can only find references to __int128 and __int128_t

Testing with the latest clang & gcc, the only int128-related define is SIZEOF_INT128:

% echo "#include <stdint.h>" | clang-18 -dM -E - | grep -i int128 #define __SIZEOF_INT128__ 16 % echo "#include <stdint.h>" | gcc-14 -dM -E - | grep -i int128 #define __SIZEOF_INT128__ 16

Apparently this is defined from gcc >= 4.6 and clang >= 3.3.

Quoting the C23 standard:

7.22.1.1 Exact-width integer types

1 The typedef name intN_t designates a signed integer type with width N and no padding bits. Thus, int8_t denotes such a signed integer type with a width of exactly 8 bits.
2 The typedef name uintN_t designates an unsigned integer type with width N and no padding bits. Thus, uint24_t denotes such an unsigned integer type with a width of exactly 24 bits.
3 If an implementation provides standard or extended integer types with a particular width and no padding bits, it shall define the corresponding typedef names.

J.6.1 Rule based identifiers
1 The following 53 regular expressions characterize identifiers that are systematically reserved by some clause in this document.
[...] int[a-zA-Z0-9_]*_t, [...] uint[a-zA-Z0-9_]*_t

This makes int128_t and uint128_t reserved names.

For some reason, neither gcc not clang define int128_t or uint128_t eventhough they do support 128-bit integer arithmetics, but don't accept 128-bit integer literals and the standard library does not support 128-bit printf or scanf conversions, albeit there is a way to specify these in the format string: %w128d, %w128u, etc.

Using different identifiers would avoid potential potability issues and testing for INT128_MAX would be a portable way to support 128-bit arithmetics on targets that have standard support for it.

This makes int128_t and uint128_t reserved names.

Ah apologies, I misunderstood.

For the current changes, should I also add the change to the bf_ identifiers like below:

#ifndef INT128_MAX __extension__ typedef __int128 int128_t; __extension__ typedef unsigned __int128 uint128_t; #endif typedef int128_t bf_int128_t; typedef uint128_t bf_uint128_t;

Or was there a different naming scheme for the bf_ prefix that you'd prefer? I can also handle that in a separate PR if that's easier

Actually, only add the #ifndef guard. Do not bother defining the bf_xxx types as this would create too many changes in both libbf.c and quickjs.c. This module is going to change to keep minimal changes.

No worries, current changes should be good to go then

chqrlie · 2024-06-23T15:23:47Z

libregexp.c

-    void *buf[0];
+    void *buf[];


We did not use the standard flexible array syntax because it was not supported by older compilers on some targets, possibly in the embedded world. We do use compound literals so it is unlikely for the compiler to support this and not that. All targets in the CI seem happy so I would OK this change.
An alternative would be to use a macro FLEXIBLE defined as nothing on compliant targets and 0 or even 1 on those that do not support the standard syntax.

quickjs.c

saghul · 2024-06-24T08:01:14Z

What happens if we set https://cmake.org/cmake/help/latest/variable/CMAKE_C_EXTENSIONS.html#variable:CMAKE_C_EXTENSIONS ?

Right now we are using c11 rather than gnu11 (in CMake terms). Would -Wpedantic still give warnings if we set the standard to gnu11?

saghul · 2024-06-24T08:03:23Z

Nevermind, we already do (I missed it) and -Wpedantic still gives the warnings.

andrjohns added 5 commits June 23, 2024 15:07

Use ISO C syntax for flexible array members

5a6dea3

Mark __int128 as extension

6d628d6

Forward reference to enum type

acc7758

Avoid void pointer to function pointer casts

4b5e033

Mark indirect goto and address-as-label as extensions

a28c53c

chqrlie requested changes Jun 23, 2024

View reviewed changes

Remove unnecessary forward reference for OpCodeEnum

5f6ec19

saghul reviewed Jun 23, 2024

View reviewed changes

quickjs.c Show resolved Hide resolved

andrjohns added 2 commits June 24, 2024 14:28

Guard int128_t typedef

43b4edf

Add recommended workaround for dlsym void* return cast

da0746b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid warnings when compiling under `Wpedantic` #437

Avoid warnings when compiling under `Wpedantic` #437

andrjohns commented Jun 23, 2024

chqrlie left a comment

chqrlie Jun 23, 2024

chqrlie Jun 23, 2024

andrjohns Jun 23, 2024

chqrlie Jun 24, 2024 •

edited

Loading

chqrlie Jun 24, 2024 •

edited

Loading

andrjohns Jun 24, 2024

chqrlie Jun 24, 2024

andrjohns Jun 24, 2024

chqrlie Jun 24, 2024

andrjohns Jun 24, 2024

chqrlie Jun 23, 2024

saghul commented Jun 24, 2024

saghul commented Jun 24, 2024

Avoid warnings when compiling under Wpedantic #437

Are you sure you want to change the base?

Avoid warnings when compiling under Wpedantic #437

Conversation

andrjohns commented Jun 23, 2024

chqrlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chqrlie Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

chqrlie Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul commented Jun 24, 2024

saghul commented Jun 24, 2024

Avoid warnings when compiling under `Wpedantic` #437

Avoid warnings when compiling under `Wpedantic` #437

chqrlie Jun 24, 2024 •

edited

Loading

chqrlie Jun 24, 2024 •

edited

Loading