Re: [lmi] [lmi-commits] master d810372 3/9: Trade elegance for simplicity

[Vadim Zeitlin]

On Sat, 23 Oct 2021 18:32:44 -0400 (EDT) Greg Chicares 
<gchicares@sbcglobal.net> wrote:

GC> branch: master
GC> commit d8103724347db1cf4945adeb5fb9dec4e0a8bf23
GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> 
GC>     Trade elegance for simplicity

 Sorry, I probably should have explained it better, but it can't be that
simple because this version doesn't work, as can be seen by the errors at

        https://github.com/let-me-illustrate/lmi/runs/3986134857

GC>     $(gcc_cxx_warnings) seems adequate for gcc version-specific warnings:

 Unfortunately it isn't. And yes, this means that the existing assignment
to it doesn't work neither, i.e. -Wredundant-tags and -Wvolatile are never
used.

GC>     $(gcc_version_specific_warnings) hasn't been used since gcc-7.

 I've used this one only for consistency with the gcc 10 case and I don't
care at all if it is removed. But using gcc_cxx_warnings like it is used in
this commit doesn't work and needs to be changed.

GC> diff --git a/workhorse.make b/workhorse.make
GC> index b0343f4..a9e5795 100644
GC> --- a/workhorse.make
GC> +++ b/workhorse.make
GC> @@ -488,10 +488,12 @@ else ifneq (,$(filter $(gcc_version), 10 10.0))
GC>  
GC>    cxx_standard := -fno-ms-extensions -frounding-math -std=c++20
GC>  else ifneq (,$(filter $(gcc_version), 11 11.0))
GC> -  gxx_version_specific_warnings := \
GC> -    -Wno-deprecated-enum-float-conversion \
GC> +  gcc_version_specific_warnings := \
GC>  
GC> -  gcc_cxx_warnings += -Wredundant-tags -Wvolatile
GC> +  gcc_cxx_warnings += \
GC> +    -Wno-deprecated-enum-float-conversion \
GC> +    -Wredundant-tags \
GC> +    -Wvolatile \
GC>  
GC>    cxx_standard := -fno-ms-extensions -frounding-math -std=c++20
GC>  endif

 The problem is that setting gcc_cxx_warnings here doesn't work because ...

GC> @@ -571,7 +573,6 @@ gcc_c_warnings := \
GC>  gcc_cxx_warnings := \
GC>    $(cxx_standard) \
GC>    $(gcc_common_warnings) \
GC> -  $(gxx_version_specific_warnings) \
GC>    -Wc++11-compat \
GC>    -Wc++14-compat \
GC>    -Wc++1z-compat \

... it's overwritten by the assignment to it below.

 The simplest would be to revert this commit and restore my -- slightly
more complicated, but working -- version. Otherwise we'd need to replace
the ":=" in the latest assignment with "+=", but I didn't want to do this
because it would make it inconsistent with gcc_c_warnings. It also looks
like it was intended to be defined only once, using helper variables such
as cxx_standard and gcc_common_warnings, so adding another such helper
seemed like a reasonable thing to do.

 To be honest, I still don't see anything wrong with my original approach
and I'd just replace the existing "gcc_cxx_warnings +=" lines with it too
(in fact, I was going to suggest just this, but wanted to do it after this
PR was merged to avoid mixing up several different things, but, in
hindsight, it would have clearly been better to deal with this at the same
time). But if you see a better solution, I'm sure I'd be perfectly fine
with it too -- it's just that something needs to be done to actually use
the newly added option, because currently it isn't used and the CI build
still fails.

 Thanks in advance,
VZ

pgpkD7LDUlMD4.pgp
Description: PGP signature