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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Aug 31 2021, Carlo Arenas wrote:

> On Mon, Aug 30, 2021 at 4:40 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Mon, Aug 09 2021, Phillip Wood wrote:
>> > On 09/08/2021 02:38, Carlo Marcelo Arenas Belón wrote:
>> >> 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.
>
>> 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/. 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.