Re: [PATCH/RFC 3/3] ci: run a pedantic build as part of the GitHub workflow

On Tue, Aug 31 2021, Carlo Arenas wrote:

On Mon, Aug 30, 2021 at 4:40 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Mon, Aug 09 2021, Phillip Wood wrote:

quoted

On 09/08/2021 02:38, Carlo Marcelo Arenas Belón wrote:

quoted

similar to the recently added sparse task, it is nice to know as early
as possible.
add a dockerized build using fedora (that usually has the latest
gcc)
to be ahead of the curve and avoid older ISO C issues at the same time.

If we want to be able to compile with -Wpedantic then it might be
better just to turn it on unconditionally in config.mak.dev. Then
developers will see any errors before they push and the ci builds will
all use it rather than having to run an extra job. I had a quick scan
of the mail archive threads starting at [1,2] and it's not clear to me
why -Wpedaintic was added as an optional extra.

This is from wetware memory, so maybe it's wrong: But I recall that with
DEVOPTS=pedantic we used to have a giant wall of warnings not too long
ago (i.e. 1-3 years), and not just that referenced
USE_PARENS_AROUND_GETTEXT_N issue.

when gcc (and clang) moved to target C99 by default (after version 5)
then that wall of errors went away.  Indeed git can build cleanly in a
strict C99 compiler and until reftable was able to build even with gcc
2.95.3

the nostalgic can get it back with `CC=gcc -std=gnu89`, and indeed I
was considering this might be a good alternative to the defunct
gcc-4.8 job, where the weather balloons breaking with strict C89
compatibility could be explicitly coded.

quoted

So if we turn pedantic on in DEVOPTS by default, wouldn't it make sense
to at least have a CI job where we test that we compile with
USE_PARENS_AROUND_GETTEXT_N (which at that point would not be the default
anymore).

agree, and indeed was thinking it might be worth combining this job
with the SANITIZE one for efficiency.

On the other hand maybe we should just remove
USE_PARENS_AROUND_GETTEXT_N entirely, i.e. always use the parens.

That facility seems to have been added in response to a one-off mistake
in 9c9b4f2f8b7 (standardize usage info string format, 2015-01-13). See
https://lore.kernel.org/git/ecb18f9d6ac56da0a61c3b98f8f2236@74d39fa044aa309eaea14b9f57fe79c/ (local). That
later landed as 290c8e7a3fe (gettext.h: add parentheses around N_
expansion if supported, 2015-01-11).

It doesn't seem worth the effort to forever maintain this special case
and use CI resources etc. to catch what was effectively a one-off typo.