[v3,0/3] support pedantic in developer mode

Message

Carlo Marcelo Arenas Belón Sept. 3, 2021, 5:02 p.m. UTC

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/[e-mail]/

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

 Makefile                     | 22 ++--------------------
 compat/nedmalloc/nedmalloc.c |  2 +-
 compat/win32/lazyload.h      |  2 +-
 config.mak.dev               | 19 +++++++++++--------
 gettext.h                    | 24 ------------------------
 git-compat-util.h            |  4 ----
 6 files changed, 15 insertions(+), 58 deletions(-)

--
v3
- replace the first patch with an even better worded one from Ævar
- include minor changes needed for Windows
- version check new flags to avoid risk of breaking old compilers
- drop alternative
v2
- enable pedantic globally instead of single job as suggested by Phillip
- propose an alternative solution for USE_PARENS_AROUND_GETTEXT_N
v1
- create job to check for pedantic compilation (only in Linux, using
  Fedora)

2.33.0.481.g26d3bed244

Comments

Ævar Arnfjörð Bjarmason Sept. 5, 2021, 7:54 a.m. UTC | #1

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

> 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/[e-mail]/
>
> 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.