Re: [lmi] [lmi-commits] master 1372a42 6/7: Use '-isystem' to suppress warnings in third-party headers

[Greg Chicares]

On 2020-06-08 17:04, Vadim Zeitlin wrote:
> On Mon, 8 Jun 2020 16:13:15 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> if some MACRO from a 3rd party header resulted in a warning, we could do
> 
>       #define LMI_MACRO(args) \
>               _Pragma("GCC diagnostic push") \
>               _Pragma("GCC diagnostic ignored \"-Wwhatever\"") \
>               MACRO(args) \
>               _Pragma("GCC diagnostic pop")
> 
> and use LMI_MACRO() in the code instead.
> 
>  Now that I think about it, I realize that we actually should do this
> inside wxWidgets EVT_XXX macros themselves.

Yes, it's much better to change one library's sources than to
change each application that uses the library.

> Are there any other macros that
> are problematic from this point of view?

These two errors leak through even with '-isystem /path/to/wx':

/opt/lmi/src/lmi/census_view.cpp: In constructor 
‘wxFloatingPointValidator<T>::wxFloatingPointValidator(wxFloatingPointValidator<T>::ValueType*,
 int) [with T = double; wxFloatingPointValidator<T>::ValueType = double]’:
/opt/lmi/src/lmi/census_view.cpp:297:38: error: zero as null pointer constant 
[-Werror=zero-as-null-pointer-constant]
     wxFloatingPointValidator<double> val;
                                      ^~~
/opt/lmi/src/lmi/skeleton.cpp: In member function ‘void 
wxEvtHandler::Bind(const EventTag&, void (Class::*)(EventArg&), EventHandler*, 
int, int, wxObject*) [with EventTag = wxEventTypeTag<wxMenuEvent>; Class = 
Skeleton; EventArg = wxMenuEvent; EventHandler = Skeleton]’:
/opt/lmi/src/lmi/skeleton.cpp:749:68: error: zero as null pointer constant 
[-Werror=zero-as-null-pointer-constant]
         frame_->Bind(wxEVT_MENU_OPEN, &Skeleton::UponMenuOpen, this);
                                                                    ^

And of course it's easy to find all diagnostics that are inhibited
by '-isystem' for wx, by moving one line:

 lmi_include_directories := \
   $(srcdir) \
   $(srcdir)/tools/pete-2.1.1 \
   $(overriding_include_directories) \
+  $(wx_include_paths) \

 sys_include_directories := \
   $(compiler_include_directory) \
-  $(wx_include_paths) \
   /opt/lmi/third_party/include \
   /opt/lmi/third_party/src \
   $(localincludedir) \
   $(localincludedir)/libxml2 \

and then rebuilding after 'make clobber'.

> GC>   https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html
> GC> | There is also a directive, #pragma GCC system_header, which tells GCC
> GC> | to consider the rest of the current include file a system header, no
> GC> | matter where it was found. Code that comes before the ‘#pragma’ in
> GC> | the file is not affected. #pragma GCC system_header has no effect in
> GC> | the primary source file.
> GC> 
> GC> That pragma could be added (before the first line of code) to every
> GC> third-party header file, by such means as:
> GC>  - convincing upstream maintainers to add it; or
> GC>  - forcibly inserting it, e.g. with 'sed'.
> 
>  There is an additional problem with this approach, which also affects
> "-isystem" one: I can imagine cases when a warning in a 3rd party header
> would be _desirable_, i.e. when it would be given because the API is used
> incorrectly.

Can you give an example? I'm puzzled because I expect C++ to warn
if I use a type-safe API incorrectly, even if it's supplied by a
"system" header. For instance,
  std::memset(1,2,3,"hello world);
ought to be diagnosed, even though std::memset()'s header is
already a gcc system header.

Or are you thinking of violations of a macro's "API"? This
  TWO_ARGUMENT_MACRO("unacceptable", "arguments")
isn't type-safe, but...well...is there a concrete example where
the '-isystem' treatment would prevent diagnosing a real error?

Or are you thinking of internal-consistency violations in a
third-party library, which I'd argue it's not the library user's
job to find? (For a reasonable library like wx, we could build
with the move-one-line patch above, periodically; for ancient
libraries, what would be the point?)

> GC> Anyway, my plan is to continue pursuing the '-isystem' method, and
> GC> then go back later and consider whether there's a better way.
> 
>  I think using pragmas is a better way if we can solve the problem with the
> macros as proposed above.

A pragma solves a specific problem in a specific way; no other
method can match its specificity. But pragmata are ugly; over
time, they can become difficult to maintain; and, worst of all,
they breed. The counterpoint is that
 - library errors should be fixed in the library, rather than
   worked around in every application that uses it; and,
   failing that,
 - '-isystem' is a terse, general solution when all we want to
   do is to exclude warnings about library issues from the
   output resulting from application builds.