Re: [lmi] PATCH: tests build fixes for clang

[Vadim Zeitlin]

On Mon, 8 Mar 2021 22:57:11 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 3/8/21 6:39 PM, Vadim Zeitlin wrote:
GC> > On Sun, 7 Mar 2021 23:13:01 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:
GC> > 
GC> > GC> It looks like we once suppressed many warnings in 'pchfile_wx.hpp',
GC> > GC> and have progressively suppressed the suppression of most.
GC> 
GC> Of course, it's confusing that some warnings are disabled in a
GC> PCH file--even, I guess, for compilers with which we don't
GC> actually precompile headers--while others are disabled in a
GC> makefile. The reason for this chimerical approach was:
GC> 
GC> // Even if precompiled headers are not really being used, use this header to
GC> // disable some warnings which are enabled for the rest of lmi code but 
have to
GC> // be disabled for the code using wxWidgets as they occur in wxWidgets 
headers.
GC> 
GC> Formerly, that comment pertained to numerous suppressed warnings,
GC> but now it's used only for 'Wzero-as-null-pointer-constant'. If
GC> that's no longer needed for wx, we can slay the chimera.
GC> 
GC> > I'll take
GC> > GC> it on faith that the last remaining one is a gcc-8 defect; in that
GC> > GC> case, conditionalizing it on the gcc version sounds ideal to me.
GC> > 
GC> >  We can see that gcc-9 doesn't give these warnings in the CI build at
GC> > 
GC> > 
https://github.com/let-me-illustrate/lmi/pull/173/checks?check_run_id=2051098965
GC> > 
GC> > and I also don't see these particular warnings locally with gcc-10.
GC> > However, I do see the same warnings in include/xmlwrapp/node.h when
GC> > building using configure, but they're actually justified there and the
GC> > error message correctly points to several places where pointers are
GC> > initialized using literal "0". I've corrected this in xmlwrapp itself, but
GC> > I haven't updated the lmi version because somehow they're not given when
GC> > using lmi makefiles.
GC> > 
GC> >  The trouble is that I really don't understand why aren't they.
GC> 
GC> I think this is why:
GC> 
GC> i686-w64-mingw32-g++ -MMD -MP -MT input_xml_io.o -MF input_xml_io.d  -c
GC>   -I /opt/lmi/src/lmi
GC>   -I /opt/lmi/src/lmi/tools/pete-2.1.1
GC>   -isystem 
/opt/lmi/local/gcc_i686-w64-mingw32/lib/wx/include/i686-w64-mingw32-msw-unicode-3.1
GC>   -isystem /opt/lmi/local/include/wx-3.1
GC>   -isystem /opt/lmi/third_party/include
GC>   -isystem /opt/lmi/local/include
GC>   -isystem /opt/lmi/local/include/libxml2
GC>   ...
GC> 
GC> Spoiler below.

 Thanks, I've indeed completely forgotten about -isystem which explains the
problem, of course. I guess I knew it would cost me time when I was arguing
against using it in the first place... But I'll try to not forget about it
any longer.

GC> >  Anyhow, to return to the original plan of modifying pchfile_wx.hpp to
GC> > still disable this warning for gcc-8, but not for the later versions, I
GC> > think I have a better proposal: why not add 
-Wzero-as-null-pointer-constant
GC> > option for gcc > 8 only? We already check gcc version in workhorse.make
GC> > anyhow, so it would be simple to do and I think it would be both better
GC> > (because this flag is clearly broken with gcc 8) and simpler (because we
GC> > wouldn't need to do anything at all to disable it at the code level).
GC> 
GC> I think you mean something like this:
GC> 
GC> diff --git a/workhorse.make b/workhorse.make
GC> index 8bc1f476d..a8ad6f6e6 100644
GC> --- a/workhorse.make
GC> +++ b/workhorse.make
GC> @@ -436,6 +436,7 @@ else ifneq (,$(filter $(gcc_version), 7.2.0 7.3.0))
GC>    cxx_standard := -fno-ms-extensions -frounding-math -std=c++17
GC>  else ifneq (,$(filter $(gcc_version), 8 8.1.0 8.2.0 8.3.0 9 9.3.0))
GC>    gcc_version_specific_warnings := \
GC> +    -Wno-zero-as-null-pointer-constant \
GC>  
GC>    ifeq (x86_64-w64-mingw32,$(findstring x86_64-w64-mingw32,$(LMI_TRIPLET)))
GC>  # See:
GC> 
GC> which would of course need a refinement to exempt "9 9.3.0".

 Yes, something like this.

GC> But I don't think we should do anything like that, because
GC> IIRC the warning did find some actual defects with gcc-8.x .
GC> 
GC> And as soon as we upgrade to 10.x, the issue becomes moot, so
GC> any time we spend on it now will have been wasted.

 OK, I won't touch it then, I don't really need to use gcc 8 with
configure.


GC> > [*] I wonder if there is some way to avoid lmi makefiles overriding my 
make
GC> >     options by explicitly setting MAKEFLAGS. I have to remember to do
GC> >     things like "make local_options==--what-if=/full/path/skeleton.cpp" to
GC> >     force recompiling the file instead of just using --what-if normally
GC> >     because of it and this is just annoying.
GC> 
GC> Relevant excerpts from 'GNUmakefile':
[...snipped...]
GC> I don't quite understand the problem. The only 'make' option I
GC> occasionally use is '-d', and 'make -d some_target' seems to work.

 I also regularly use --dry-run and --stop (because when I introduce some
bug into a wx header of the wx version I'm testing lmi with, I really
don't want to keep getting the same error several dozen times in a row).
I very rarely use -d however.

GC> But I can reproduce it:
GC> 
GC> $make local_options=--what-if=/opt/lmi/src/lmi/skeleton.cpp $coefficiency 
install check_physical_closure 2>&1 | less -S
GC> $make --what-if=/opt/lmi/src/lmi/skeleton.cpp $coefficiency install 
check_physical_closure 2>&1 | less -S
GC> 
GC> The first command does what you want; the second doesn't.
GC> 
GC> Is the solution to change 'GNUmakefile' like this?
GC> 
GC> +MAKEFLAGS += \
GC> -MAKEFLAGS := \
GC>    --keep-going \
GC>    --no-builtin-rules \
GC>    --no-builtin-variables \
GC> 
GC> That would require testing, of course, and I haven't tested it.

 I thought this would work, but it doesn't. I have to admit that I've never
understood why do we bother with MAKEFLAGS here rather than just passing
these options directly to submake. But then, of course, I've never really
understood why do we use a submake in the first place (which complicates
things a lot, AFAICS, without any real benefit), so I'm clearly missing a
lot of things here...

VZ

pgpYJerJ5Da0E.pgp
Description: PGP signature