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] [code standardisation] Use standard C attributes #610

Closed
wants to merge 2 commits into from

Conversation

jbowler
Copy link
Contributor

@jbowler jbowler commented Oct 3, 2024

ISO have recently standardized a way of defining attributes based on the
ISO-C++ syntax standardised in 2011. This allows removal of the
existing compiler-specific hacks as well as compatibility with C++. The
latter is important for attributes used in the header files which
declare the libpng API (png.h and descendants.)

This also extends the attribute handling to any compliant C compiler,
not just the three with compiler specific support in the prior code.

The fix also enables removal of the highly idiosyncratic use of specific
comments to indicate "fall through" in case statements; the concept is
now supported by the [[fallthrough]] attribute.

The changes include checks to ensure that the attribute syntax is
supported by the compiler and that the specific attributes are also
supported.

The changes are detailed in these commits:

commit 51b6189
commit 8942be9

Signed-off-by: John Bowler jbowler@acm.org

@jbowler jbowler changed the title Use standard C attributes [libpng18] [code standardisation] Use standard C attributes Oct 3, 2024
@jbowler jbowler force-pushed the libpng18-FALLTHROUGH-fix branch 3 times, most recently from a7fb98b to f6dc7e7 Compare October 11, 2024 16:01
@jbowler
Copy link
Contributor Author

jbowler commented Oct 12, 2024

@ctruta a ping on this PR, it changes a number of files because ISO standardised a placement for the attributes which GCC does not accept as conforming to where GCC/MSVC attributes have to be place. I.e. GCC wants them in a different place. Consequently there's the potential for merge conflicts here if the change remains hanging.

@ctruta
Copy link
Member

ctruta commented Oct 12, 2024

Acknowledged, thank you. Please give me another day or two, because I want to integrate my own changes to the CI scripts (so that we can have verification both from libpng16 and libpng18) and to the CMake file (so that I can expand my testing, including the cross-platform testing). I plan on releasing libpng-1.6.45 with the most recent changes, BTW.

But I won't be changing any png*.[ch] source code, so your PRs should apply cleanly afterwards, no further rebasing necessary.

ISO have recently standardized a way of defining attributes based on the
ISO-C++ syntax standardised in 2011.  This allows removal of the
existing compiler-specific hacks as well as compatibility with C++.  The
latter is important for attributes used in the header files which
declare the libpng API (png.h and descendants.)

This also extends the attribute handling to any compliant C compiler,
not just the three with compiler specific support in the prior code.

The fix also enables removal of the highly idiosyncratic use of specific
comments to indicate "fall through" in case statements; the concept is
now supported by the [[fallthrough]] attribute.

The changes include checks to ensure that the attribute syntax is
supported by the compiler and that the specific attributes are also
supported.

The changes are detailed in these commits:

commit 51b6189
commit 8942be9

Signed-off-by: John Bowler <jbowler@acm.org>
pngwrite.c Outdated Show resolved Hide resolved
Signed-off-by: John Bowler <jbowler@acm.org>
@jbowler jbowler marked this pull request as draft October 17, 2024 16:00
@ctruta
Copy link
Member

ctruta commented Oct 18, 2024

@ctruta a ping on this PR [...] there's the potential for merge conflicts here if the change remains hanging.

I kept thinking about this PR, as well as #572. I want to reiterate that I do want to take this stuff in, but I'd like to refrigerate it for now.

I said before that the order in which I'm applying external contributions (as well as my own changes) is somewhat chaotic, but there is at least some logic to it. We need this commit, but we need house cleaning also, and I'd like to prioritize the house cleaning first. (Better develop on the smaller and cleaner codebase.) Another thing that I'd like to have is a smaller divergence between the branches libpng16 and libpng18, at least for the beginning of the life of libpng18, because the upcoming fixes for libpng18 that will need to be applied to libpng16 will be that much easier to apply and cleaner also.

So, after I publish libpng-1.6.45, I'd like to resync the two branches, and then (when possible) to use git merge COMMIT instead of git cherry-pick COMMIT, just to keep the two branches closer to one another. Don't fret too much about merging the conflicts, for now, @jbowler. After we're (mostly) done with the above-mentioned priorities, we'll come back to this one.

@jbowler jbowler closed this Oct 18, 2024
@jbowler jbowler deleted the libpng18-FALLTHROUGH-fix branch October 18, 2024 21:55
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