Build with -Wpedantic and -Werror

[brettviren]

Basic problem

We want to build WCT with strict compiler checks and so -Wpendantic and -Werror are turned on by default.

While we can WCT code itself "clean" this leads to problems when code that is #include is itself not "clean" and that will break building with -Wpedantic and -Werror.

Problem likely to reoccur over time

This issue start to fix a specific instance of this class of problem. However, the problem can reappear as novel versions of existing externals are used to build WCT by the user. Feel free to re-open this issue when those are encountered.

Basic fix

To have our strict compilation while relaxing it for offending external code we must use a non-standard but effectively universal CPP macro #pragma.

To apply the fix in general, follow this recipe:

1. Identify the problematic header from compiler errors, which will give the full #include pagth.
2. Find where these headers are used in the source via grep or your favorite tool.
3. Identify a suitable "gatekeeper header" or make a new one (see below).
4. Include the offending headers wrapped in #pragma (see below)
5. Remove/replace direct #include of the offending headers in other source and #include the "gatekeeper header".

A "gatekeeper header" is a header file that includes some offending header(s) of some limited set and wraps them in suitable #pragma to turn off the strict compiling. This "gatekeeper header" is then used for #include instead of including the "bare" offending headers.

The initial problem

In this inaugural case the two offending headers show up when Boost 1.75.0 and Eigen 3.4.0 are used with GCC 9.3.0.

#include <boost/iostreams/detail/functional.hpp>
#include <Eigen/SparseCore/SparseMatrixBase.h>

These are not directly included but are included by other's in Boost and Eigen, respectively.

WCT currently includes boost/iostreams related in many places and for different usages. So, it a new "gatekeeper header" for just these is warranted. Let's call that WireCellUtil/Iostreams.h in analogy to the existing MultiArray.h. OTOH, the use of Eigen can likely always go through the Array.h header and so we can instrument that as the "gatekeeper".

Because the build turns on -Werror, anything that would merely be a warning becomes an error. This requires two #pragma for each actual warning. Here is the guts of MultiArray.h showing things:

#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wdeprecated-declarations"
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#pragma GCC diagnostic warning "-Wmaybe-uninitialized"
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
#pragma GCC diagnostic warning "-Warray-bounds"
#pragma GCC diagnostic ignored "-Warray-bounds"

#include <boost/multi_array.hpp>

#pragma GCC diagnostic pop

The last chore is to replace #include of bare offending headers with inclusion of the gatekeeper. Again, grep or your favorite search mechanism will help find the spots needing editing.

[HaiwangYu]

Hi @brettviren , thanks for making this issue describing and tracking this problem.

Here I attach an example log file for demonstration purpose:
log.txt
The warnings in this log are specific to Boost 1.75.0 and Eigen 3.4.0 and with GCC 9.3.0 as Brett mentioned.

These are fixed with this commit: f63acbf using the above #pragma method described by Brett.

I fully agree that in the long run, we want to have gatekeepers to avoid too many sprinkled #pragma. But I think we can do it later.

[brettviren]

Related #122

[HaiwangYu]

For the "gatekeepers", could @brettviren suggest what is a proper granularity?
Meaning should we have individual gatekeeper.h for each individual boost.h or put all the boost.h in one?

[brettviren]

I see it as a balance between two extremes.

• include into any given translation unit only the headers needed.
• minimize the number of gatekeeper headers.

The first extreme is one gatekeeper per offending header and the other is one gigantic include-everything gatekeeper.

The extremes should be avoided but otherwise I feel broad optimum. A good rule of thumb is a gatekeeper should not include headers from more than one external package. In the case of Boost, I've tried to narrow to one Boost "topic", eg MultiArray.h.

In the past I have looked at where headers from one external package are included to try to find patterns of use. Eg:

grep boost/iostreams **/*.h **/*.cxx

For that example, most already include Stream.h or could do so without much harm. So, we might move the inclusion of the offending headers to Stream.h and include Stream.h in their place, if not already there.

Where no existing header conveniently serves as a gatekeeper, we can of course create a new one that has only that role.