Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blink/checked.h: fix syntax error when building with GCC 9.4 #150

Merged
merged 1 commit into from
Aug 9, 2023
Merged

blink/checked.h: fix syntax error when building with GCC 9.4 #150

merged 1 commit into from
Aug 9, 2023

Conversation

tkchia
Copy link
Collaborator

@tkchia tkchia commented Aug 9, 2023

blink/checked.h:10:47: error: missing binary operator before token "("
   10 |      (defined(__has_builtin) && (__has_builtin(__builtin_add_overflow) && \
      |                                               ^

```
blink/checked.h:10:47: error: missing binary operator before token "("
   10 |      (defined(__has_builtin) && (__has_builtin(__builtin_add_overflow) && \
      |                                               ^
```
@tkchia tkchia requested review from jart and ghaerr August 9, 2023 16:02
@tkchia
Copy link
Collaborator Author

tkchia commented Aug 9, 2023

Unfortunately it seems we simply cannot try to test for __has_builtin and try to use it as well, within a single #if directive. (And a compiler that can grok the __has_builtin(...) syntax probably already supports it anyway.)

So it is necessary to split the relevant part into two directives, like so:

#if ... defined(__has_builtin)
#if __has_builtin(__builtin_add_overflow) ...
...
#endif
#endif

I am guessing that __has_include is susceptible to the same problem — so I have also modified the use of __has_include accordingly.

Thank you!

Copy link
Collaborator

@ghaerr ghaerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @tkchia,

Well, the good news is that this all gets to live in a header file :)

Out of curiosity, how likely might it be that gcc or clang might have __builtin_mul_overflow but not __builtin_add_overflow? I'm wondering whether we're ultimately just dealing with gcc and clang version numbers or other compilers as well these days?

Thank you!

@tkchia
Copy link
Collaborator Author

tkchia commented Aug 9, 2023

Hello @ghaerr,

I am not sure, but I think it is generally a good idea for code to test directly for features they want, whenever possible, in contrast to testing version numbers. 🙂

Incidentally, the relevant C23 proposal (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2669.pdf) does mention that Microsoft Visual C has an <intsafe.h> header with a somewhat different API (https://docs.microsoft.com/en-us/windows/win32/api/intsafe). I suppose some compiler vendors might want to offer up an interface for safe integer arithmetic that is distinct from the GCC __builtin_... one. 🙂 🙂

Thank you!

@ghaerr
Copy link
Collaborator

ghaerr commented Aug 9, 2023

Hello @tkchia,

I think it is generally a good idea for code to test directly for features they want, whenever possible, in contrast to testing version numbers. 🙂

I only ask that because, in the lines immediately before the __has_builtin checks, we check for compiler version numbers!! Is that version check for gcc necessary, as we're essentially duplicating the definitions of chk_add et al below.

Thanks for the information on the safe integer proposal in C23.

Thank you!

@tkchia
Copy link
Collaborator Author

tkchia commented Aug 9, 2023

Hello @ghaerr,

My suspicion is that the test for __GNUC__ >= 5 is to cater to super-old versions of GCC that had the builtin functions but did not support __has_builtin yet.

At the same time I guess there might be (who knows?) compilers that somehow manage to support __has_builtin __builtin_add_overflow, but for some reason do not claim compatibility with some version of GCC. (OK, perhaps this combination is not particularly likely. But I see that chibicc does implement some other intrinsic functions, such as __builtin_types_compatible_p.)

Thank you!

@tkchia tkchia merged commit 1ef448d into jart:master Aug 9, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants