[libpng18] Update compile time support for target-specific code #614
Conversation
This removes the need for build configuration of target-specific code such as SIMD enhancements are machine specific coding (e.g. Intel's crc32 instruction). The extensible framework based on checks in the new pngtarget.h allows per-target description of requirements and capabilities using PNG_TARGET_ macros defined by the core code and per-target implementation within, currently, a single core file pngsimd.c This ensures that no special configuration in the build system is required on any system which is compiling for the build host (where we can reasonably assume that the compiler is correct set up) or for cross-builds so long as the toolchain - the cross compiler and any ancilliary programs it requires - are correct set up. See pngtarget.h and pngsimd.c for more documentation. Signed-off-by: John Bowler <[email protected]>
The completes verification of the VSX optimization build and corrects the declaration of the "init" function. Signed-off-by: John Bowler <[email protected]>
The code issues -Wshadow, -Wcast-align and -Wunused warnings. The actual check on the ISA revision seems to be excessive; the code does compile at ISA level 2 and probably the ISA check is unnecessary. Built but cannot be tested without an emulator or hardware. Signed-off-by: John Bowler <[email protected]>
jbowler
changed the title
|
@ctruta this should be good to go. |
There was a problem hiding this comment.
The code looks good. I don't have actual review objections, only nitpicking.
After you address your comments, feel free to add the "Reviewed-by:" line before the "Signed-off-by:" line.
About nitpicking: for the first line of the commit message, I would prefer a full sentence in present tense. For example: "Add full compile-time support for architecture-specific code"
And for the last line in the commit message: that "see pngfoo.h and pngblah.c" line might be appreciated by Captain Obvious, but (in my professional experience) short and sweet commit message that go straight to the point about what exactly has been changed (and also explain why if necessary) are more effective, especially when a reader navigates over a long and hairy Git history.
There was a problem hiding this comment.
I approve this commit, but I would prefer the label of arch-specific commits to follow the current practice of naming the actual arch (for now -- until we change this practice wholesale).
I would suggest a one-liner like the following:
powerpc: Fix a leaky function declaration
BTW, if it changes the API or the functionality in any way, it is no longer a "chore".
Another thing that needs not go in commit messages in general -- explanations about the testing process -- unless the commit itself is about the testing process. (I'm doing that myself, sometimes, in my own CI commits, so I'm partly to blame here.)
There was a problem hiding this comment.
For the commit message, I have the same comment as before: use a "semi-standardized" label for the subject line, like this:
mips: Fix specific gcc/clang warnings
and then explain, in the subsequent line, that you wanted to pacify -Wshadow, -Wcast-align and -Wunused.
Another nitpicking, if you please don't mind:
unless what you're fixing is an actual compiler error issued by an actual C language standard violation, you shouldn't say that you're fixing the compilation, but rather, that you're pacifying a compiler warnings. Those who use -Werror will know what you mean, while those who don't will not be bothered.
Exception: whenever we want to fix something that breaks the CI verification (as opposed to an actual compiler error) the formal incantation is "fix verification". But it has to be publicly verifiable, not just an internal setup.
(FYI: in my own workflow, I'm running extended compilations with extended compiler warnings enabled as well.)
jbowler
changed the title
Signed-off-by: John Bowler <[email protected]>
|
I assume you will do a squash commit here. So I just changed the subject line of the messages. I've never edited a commit message; I just do squash merges... |
jbowler
changed the title
|
Ok. I am unable to resolve the last "Changes requested" so I've incorporated all your review comments into #615 and am closing this. |
These changes are the body of the previously discussed "target specific code"
cleanup.
There are several related elements:
Move (copy'n'paste) all the code including compile time checks for specific
targets into the existing target-specific sub-directories (intel, arm etc)
so that each sub-directory contains just the code for that specific target.
This involves creating a new file "check.h" in each target-specific
directory which is used to enumerate what the target code supports (by
setting appropriate #defines).
Add two new files, pngtarget.h and pngsimd.c (following @ctruta's suggestion
for the name of the latter) which include the code from the target
directories. pngtarget.h includes the "check.h" files and builds up
information for use in the main build. pngsimd.c then uses the information
from pngtarget.h to include the appropriate implementation code.
Neither pngtarget.h nor pngsimd.c are themselves machine specific.
Make the ARM-NEON target specific code which was implemented using #defines
and extra structure members scattered through the main code independent of
a specific target optimization and isolate it so that, like the filter code,
it can be implemented for any target, not just ARM.
The result is considerable simplification of the main libpng code and
significant simplification of the target-specific code (compare arm/check.h
with the original code in pngpriv.h and arm/arm_init.c for example.)