Re: [PATCH v3 0/3] support pedantic in developer mode

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 5, 2021, 07:54 UTC
Message-ID: <87o897pi7x.fsf@evledraar.gmail.com>
In-Reply-To: <20210903170232.57646-1-carenas@gmail.com>

On Fri, Sep 03 2021, Carlo Marcelo Arenas Belón wrote:

Show 23 quoted lines

> This series enables pedantic mode for building when DEVELOPER=1 is
> used and as an alternative to only enabling it in one CI job, that
> was merged to "seen" as part of cb/ci-build-pedantic.
>
> The second patch is really an independent prerequisite to ensure
> that it doesn't break the build for Windows and is the minimal change
> possible.
>
> Additional changes needed for the git-for-windows/git fork main to be
> posted independently.
>
> It merges and builds successfully all the way to "seen" IF the known
> problem reported earlier[1] and expected as part of a reroll of 
> jh/builtin-fsmonitor is merged first.
>
> [1] https://lore.kernel.org/git/20210809063004.73736-3-carenas@gmail.com/
>
> Carlo Marcelo Arenas Belón (2):
>   win32: allow building with pedantic mode enabled
>   developer: enable pedantic by default
>
> Ævar Arnfjörð Bjarmason (1):
>   gettext: remove optional non-standard parens in N_() definition

This whole series looks good to me, thanks for picking up my patch as
the 1/3. The only comment I have on it (doesn't need a re-roll) is that
I found the first paragraph in 2/3 slightly confusing, i.e.:
    
    In preparation to building with pedantic mode enabled, change a couple
    of places where the current mingw gcc compiler provided with the SDK
    reports issues.

With "the SDK" we're talking about the Win32 SDK, which is implicit from the subject line. I'd find something like this less confusing:

    In preparation for building with DEVOPTS=pedantic enabled
    everywhere, change a couple of places where we'd get Win32 breakes
    under the GCC version provided wit hthe current MinGW version.

Or something. I'm not sure if this /only/ impacts Win32, or just that compiler version. Some of the diffstat is win32-only, but nod nedmalloc, but I see there's some parallel discussion about whether that's in effect win32-specific.

Anyway, that's all a tiny nit. In general I like the change. I also checked that an existing DEVOPTS=pedantic wouldn't accidentally enable DEVOPTS=no-pedantic (i.e. that it wasn't a glob), but it doesn't, since that's not how $(filter) works.