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

curl-compilers.m4: for gcc + want warnings, set gnu89 standard #9542

Closed
wants to merge 3 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Sep 20, 2022

To better verify that the code is C89

@bagder bagder added the build label Sep 20, 2022
@MarcelRaad
Copy link
Member

MarcelRaad commented Sep 20, 2022

This will be a problem with third-party libraries using features from later standards, like C++ comments, in their public headers. Maybe add it to some CI builds instead?

@bagder
Copy link
Member Author

bagder commented Sep 20, 2022

yeah, but more importantly this identifies numerous places within libcurl itself that are not proper C89. That was my primary thought with this, to try it out. It might not make sense to land in the short term.

The question also comes down to: are those nits worth fixing if nobody has suffered from any problems with them while we are slowly considering bumping to C99 in a future...

@bagder bagder force-pushed the bagder/gcc-gnu89 branch 2 times, most recently from f51f81a to a4ae62d Compare Sep 21, 2022
@bagder
Copy link
Member Author

bagder commented Sep 21, 2022

I decided they are probably worth fixing because:

If (and this has not been decided yet) we change the requirement to C99 at some point, then whatever is in git before that switch will be the last C89 compliant curl code and it could be worth having that in a fine shape. Compliance wise.

@monnerat
Copy link
Contributor

monnerat commented Sep 21, 2022

FYI:
I did the same kind of test locally with:

CFLAGS="... -std=C89 ..."
CPPFLAGS="... -D_XOPEN_SOURCE=700 ..."

This exhibited problems fixed (sucessfully) in 6fbf7d3, 9eec754 and c3e634d: no compiler message for the current master.
Of course, not everything was enabled in my local test.

I had to define _XOPEN_SOURCE to enable definition of some library functions. There are configure tests for most of the latter (i.e.: HAVE_FCHMOD), but it seems AC_CHECK_FUNCS does not use our predefined CFLAGS while checking.
A partular case is function kill(): it is used unconditionally by curl but its definition is disabled by -std=C89. There are probably a lot of other external definitions hidden by -std=C89 but re-enabled by -D_XOPEN_SOURCE=700.

As a consequence we might question ourselves if curl's C89 requirement only targets the language or also the libraries.

@dfandrich
Copy link
Collaborator

dfandrich commented Sep 21, 2022

@monnerat
Copy link
Contributor

monnerat commented Sep 21, 2022

-D_XOPEN_SOURCE=600 was not enough for me: strndup requires 700.

@monnerat
Copy link
Contributor

monnerat commented Sep 21, 2022

C89 doesn't provide nearly enough of an API to build curl (e.g. sockets)

Agreed: see the comment about kill() above. There might be others.
But the sockets declarations are not conditioned on POSIX/XOPEN, although not in the standard C library. In fact, they were already present in 4.2BSD (1983).

@bagder
Copy link
Member Author

bagder commented Sep 22, 2022

it's safe to assume that we're talking about language features only.

Yes exactly: language features only. Everything else we can detect and adapt to.

@bagder bagder closed this in b23ce2c Sep 23, 2022
@bagder bagder deleted the bagder/gcc-gnu89 branch Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants