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

[Greg Chicares]

On 3/8/21 6:39 PM, Vadim Zeitlin wrote:
> On Sun, 7 Mar 2021 23:13:01 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> It looks like we once suppressed many warnings in 'pchfile_wx.hpp',
> GC> and have progressively suppressed the suppression of most.

Of course, it's confusing that some warnings are disabled in a
PCH file--even, I guess, for compilers with which we don't
actually precompile headers--while others are disabled in a
makefile. The reason for this chimerical approach was:

// Even if precompiled headers are not really being used, use this header to
// disable some warnings which are enabled for the rest of lmi code but have to
// be disabled for the code using wxWidgets as they occur in wxWidgets headers.

Formerly, that comment pertained to numerous suppressed warnings,
but now it's used only for 'Wzero-as-null-pointer-constant'. If
that's no longer needed for wx, we can slay the chimera.

> I'll take
> GC> it on faith that the last remaining one is a gcc-8 defect; in that
> GC> case, conditionalizing it on the gcc version sounds ideal to me.
> 
>  We can see that gcc-9 doesn't give these warnings in the CI build at
> 
> https://github.com/let-me-illustrate/lmi/pull/173/checks?check_run_id=2051098965
> 
> and I also don't see these particular warnings locally with gcc-10.
> However, I do see the same warnings in include/xmlwrapp/node.h when
> building using configure, but they're actually justified there and the
> error message correctly points to several places where pointers are
> initialized using literal "0". I've corrected this in xmlwrapp itself, but
> I haven't updated the lmi version because somehow they're not given when
> using lmi makefiles.
> 
>  The trouble is that I really don't understand why aren't they.

I think this is why:

i686-w64-mingw32-g++ -MMD -MP -MT input_xml_io.o -MF input_xml_io.d  -c
  -I /opt/lmi/src/lmi
  -I /opt/lmi/src/lmi/tools/pete-2.1.1
  -isystem 
/opt/lmi/local/gcc_i686-w64-mingw32/lib/wx/include/i686-w64-mingw32-msw-unicode-3.1
  -isystem /opt/lmi/local/include/wx-3.1
  -isystem /opt/lmi/third_party/include
  -isystem /opt/lmi/local/include
  -isystem /opt/lmi/local/include/libxml2
  ...

Spoiler below.

>  Anyhow, to return to the original plan of modifying pchfile_wx.hpp to
> still disable this warning for gcc-8, but not for the later versions, I
> think I have a better proposal: why not add -Wzero-as-null-pointer-constant
> option for gcc > 8 only? We already check gcc version in workhorse.make
> anyhow, so it would be simple to do and I think it would be both better
> (because this flag is clearly broken with gcc 8) and simpler (because we
> wouldn't need to do anything at all to disable it at the code level).

I think you mean something like this:

diff --git a/workhorse.make b/workhorse.make
index 8bc1f476d..a8ad6f6e6 100644
--- a/workhorse.make
+++ b/workhorse.make
@@ -436,6 +436,7 @@ else ifneq (,$(filter $(gcc_version), 7.2.0 7.3.0))
   cxx_standard := -fno-ms-extensions -frounding-math -std=c++17
 else ifneq (,$(filter $(gcc_version), 8 8.1.0 8.2.0 8.3.0 9 9.3.0))
   gcc_version_specific_warnings := \
+    -Wno-zero-as-null-pointer-constant \
 
   ifeq (x86_64-w64-mingw32,$(findstring x86_64-w64-mingw32,$(LMI_TRIPLET)))
 # See:

which would of course need a refinement to exempt "9 9.3.0".

But I don't think we should do anything like that, because
IIRC the warning did find some actual defects with gcc-8.x .

And as soon as we upgrade to 10.x, the issue becomes moot, so
any time we spend on it now will have been wasted.

>  What do you think? I.e. would you accept a PR doing it like this?
> VZ
> 
> [*] I wonder if there is some way to avoid lmi makefiles overriding my make
>     options by explicitly setting MAKEFLAGS. I have to remember to do
>     things like "make local_options==--what-if=/full/path/skeleton.cpp" to
>     force recompiling the file instead of just using --what-if normally
>     because of it and this is just annoying.

Relevant excerpts from 'GNUmakefile':

# $(MAKEFLAGS) is used instead of '.SUFFIXES': it is more powerful,
# for instance because it can automatically enforce Paul's advice
# "it's best to invoke make with the -r option".

# Suppress default rules. Instead, specify all desired rules
# explicitly.
#
# Instead of using a '.SUFFIXES:' target with no dependencies,
# specify $(MAKEFLAGS) to do everything that would do, and more.

MAKEFLAGS := \
  --keep-going \
  --no-builtin-rules \
  --no-builtin-variables \

# See the documentation in 'local_options.sh'. Including this file
# defines $(local_options), which is passed to submakefiles.

-include $(srcdir)/local_options.make
$(srcdir)/local_options.make:: ;

[Excerpts end.]

I don't quite understand the problem. The only 'make' option I
occasionally use is '-d', and 'make -d some_target' seems to work.
But I can reproduce it:

$make local_options=--what-if=/opt/lmi/src/lmi/skeleton.cpp $coefficiency 
install check_physical_closure 2>&1 | less -S
$make --what-if=/opt/lmi/src/lmi/skeleton.cpp $coefficiency install 
check_physical_closure 2>&1 | less -S

The first command does what you want; the second doesn't.

Is the solution to change 'GNUmakefile' like this?

+MAKEFLAGS += \
-MAKEFLAGS := \
   --keep-going \
   --no-builtin-rules \
   --no-builtin-variables \

That would require testing, of course, and I haven't tested it.

Anyway, here's the Spoiler: '-isystem' shuts up the warning.
At least that's my guess.