Warn if an undefined identifier is evaluated in an #if directive (enable -Wundef)

[MisterDA]

Prior art in #11263 and #11267.

FTR: The C pre-processor defines an undefined variable as being zero, so it's not strictly a bug per se (#if _MSC_VER >= 1200 is equivalent to #if defined(_MSC_VER) && _MSC_VER >= 1200). However, it's reasonable for users to expect to be able to compile their own C code with gcc/clang's -Wundef.

• Although it's a slight breaking change, maybe we should undefine Reserved_space_c_stack_link after its sole use in fiber.h? It's not even namespaced.
• I've added -Wundef to our internal flags to prevent this error from happening again. It applies to public headers (good!) and internal files (bad?).
• The late macintosh macro had been forgotten during the removal of Mac OS 9…

I've tested this change with gcc, mingw-w64 (gcc), and g++ on a stub file including all the caml/ headers.

[gasche]

Sounds reasonable to me, and a trivial change.

[xavierleroy]

I'm not convinced that the (standard-conformant usage) of giving undefined macros the "0" value for numerical comparison purposes is bad. Yes, Clang and perhaps GCC have -Wundef to warn about it, but they also have other warnings that we don't care about for the OCaml code base and include files.

On the other hand, the proposed changes to please -Wundef are rather small, and can be made smaller (see comments below), so I can go along with this PR.

[MisterDA]

Thanks all for the reviews.

Argh! __STDC_VERSION__ is always defined, the C standards say so. No defined test, please.

This change was specially to satisfy GCC users who'd write C++ or Objective-C code, include caml headers, and define -Wundef. In this mode the macro is not defined. (Same for MSVC but unrelated to the undef macro problem).

$ g++ -Wundef -x c++ -c - <<'EOF'
#ifdef __STDC_VERSION__
#warning stdc-version-defined
#else
#warning stdc-version-undefined
#endif
EOF
<stdin>:4:2: warning: #warning stdc-version-undefined [-Wcpp]

(this is also the case in extern "C" {} blocks)

[dra27]

Possible alternate way of expressing the change in gethost.c with a minor tidy-up.

[gasche]

I think that the changes are small enough that merging them now is less work than explaining to people in the future what we think about -Wundef, so I am in favor of merging. Two points remain to elucidate in my opinion:

1. Are we okay to merge with the STD_VERSION definedness check to cater for people writing C code from Objective-C (really?) and C++ and what not? I personally don't care either way. I propose to merge as-is on this point, but Xavier should feel free to disagree.
2. I haven't reviewed yet the @dra27-suggested change to the GETHOSTBY* stuff, and I am hoping that @dra27 will beat me to it.

[xavierleroy]

I'm not strongly opposed to merging this PR, just a bit skeptical about the actual usefulness.

Searching for -Wundef I stumbled on... the GNU coding standard, which mention it specially!

https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#Syntactic-Conventions

Don’t make the program ugly just to placate static analysis tools such as lint, clang, and GCC with extra warnings options such as -Wconversion and -Wundef. These tools can help find bugs and unclear code, but they can also generate so many false alarms that it hurts readability to silence them with unnecessary casts, wrappers, and other complications. For example, please don’t insert casts to void or calls to do-nothing functions merely to pacify a lint checker.

[dra27]

I think it's a reasonable goal to support users being able to build their code with -Wundef and not have the public (i.e. non-CAML_INTERNALS) parts of our headers trigger that warning. If we're supporting that goal, then I very much like being able to continuously ensure that we aren't breaking it (i.e. the adding of -Wundef to the development mode flags).

An alternative would be to use the appropriate #pragmas to turn off the warning in our own C files, but I agree with #12714 (review) that the places affected are so few that this looks like the easiest way of doing it.

Thanks, @MisterDA!

[MisterDA]

One small thing I find this macro useful for is that I may have missed a header or a particular condition to enable features from a header.

// say I get this wrong
#define _WIN32_WINNT 0xBAD
// or I forget this
#include <stdheader.h>

// but I need them for this function call
int flags = THIS_WEIRD_FLAG;
f(THIS_WEIRD_FLAG);

Without -Wundef I would have checked whether the flag was defined by compiling first with

#ifndef THIS_WEIRD_FLAG
#error "somethings's fishy"
#endif

otherwise as noted the preproc would have silently replaced the weird flag with 0.

But with -Wundef I can submit my PRs to ocaml/ocaml with more confidence!

[xavierleroy]

I agree the code is cleaner this way.

[xavierleroy]

All right, that's enough good reasons to go in the direction of this PR. Merging right now!