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

[libpng18] [build system] Require ISO-C when building libpng #603

Closed
wants to merge 1 commit into from

Conversation

jbowler
Copy link
Contributor

@jbowler jbowler commented Sep 22, 2024

Previously the build systems would accept any old compiler and those
compilers would define any old symbol or macro name. ISO (well, ANSI)
defined a strict set of symbols/macros which may be defined by the
implementation leaving the remainder for application programs.

Adding a requirement for an ISO-C compiler (any version) ensures that a
check for name clashes on one compiler (with a sufficient level of
diagnostics) works for every ISO-C compiler. In other words if it
builds on one it builds on all.

The check is in pngpriv.h; the same restrictions do not apply to code
that calls the public interface of libpng, only to the compiler used to
build ligpng.

Signed-off-by: John Bowler [email protected]

Copy link
Contributor

@ProgramMax ProgramMax left a comment

Choose a reason for hiding this comment

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

LGTM.

I agree with the intention.
Unless there is some important government contract or similar obligating pre-ISO-C support, I don't think the cost of maintaining this support is worth it.

@jbowler
Copy link
Contributor Author

jbowler commented Sep 26, 2024

Unless there is some important government contract or similar obligating pre-ISO-C support

There isn't. It was always available as a "suck it and see" option. Traditionally the response to, "I sucked and got sick" was that the maintainer would either attempt to fix it or say, "You need to use an ANSI-C compiler."

My patch removes the privilege of appealing to the maintainer. Compiles fail if the compiler fails to define __STDC__ I feel it's time. In fact I feel it is so long overdue that I'm suggesting to @ctruta, @bobfriesenhahn and everyone, well anyone, currently involved that we move to something that at the very least supports inline functions. How I hate #define :-)

@jbowler jbowler changed the title [libpng18] build system: require ISO-C when building libpng [libpng18] [build system] Require ISO-C when building libpng Oct 3, 2024
@jbowler jbowler force-pushed the libpng18-require-iso-c branch 4 times, most recently from d05cf86 to 90260b6 Compare October 13, 2024 21:25
Previously the build systems would accept any old compiler and those
compilers would define any old symbol or macro name.  ISO (well, ANSI)
defined a strict set of symbols/macros which may be defined by the
implementation leaving the remainder for application programs.

Adding a requirement for an ISO-C compiler (any version) ensures that a
check for name clashes on one compiler (with a sufficient level of
diagnostics) works for every ISO-C compiler.  In other words if it
builds on one it builds on all.

The check is in pngpriv.h; the same restrictions do not apply to code
that calls the public interface of libpng, only to the compiler used to
build ligpng.

Signed-off-by: John Bowler <[email protected]>
* standard (C89, ISO-C90) is required.
*/
#if __STDC__ != 1
# error A compiler compliant with ISO-C90 is required to build libpng
Copy link
Member

Choose a reason for hiding this comment

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

One thumb up on the principle that we're straight-up requiring a modern C compiler 👍

...and...

One thumb down on the technicality that neither __STDC__ nor __STDC_VERSION__ are mandatory for a standard-conforming C compiler (at least in my interpretation of their meaning), and that, indeed, some compilers that do support C99 and newer (e.g. Microsoft C/C++ and Borland C/C++) do not define __STDC__ 👎

I might be persuaded to accept code that checks both __STDC__ and __STDC_VERSION__, and aborts the compilation if both of these symbols are missing; but even that is debatable.

The good news is that we don't need this change: we don't accept code that caters to the pre-ANSI C compilers, and that is that. In any case, libpng16 is already pretty much cleared of pre-ANSI C constructs.

The bad news is that I need to reopen #601; but the good news to the bad news is that the fix for #601 is rather trivial.

@ctruta ctruta closed this Oct 18, 2024
@ctruta ctruta reopened this Oct 18, 2024
@ctruta
Copy link
Member

ctruta commented Oct 18, 2024

One more thing before I close this PR as WONTFIX:

I'll be happy to accept, with both of my thumbs up and both of my palms open, any code contribution that consists in removing pre-ANSI C "compatibility" noise (of whose existence I might be unaware), in libpng16 (conditionally, if it doesn't break the ABI) and in libpng18 (unconditionally).

@ctruta ctruta closed this Oct 18, 2024
@jbowler jbowler deleted the libpng18-require-iso-c branch October 18, 2024 21:54
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.

3 participants