Re: Turn on compiler warnings by default for AC_PROG_CC, AC_PROG_CXX & AC_PROG_FC

[David A. Wheeler]

I said:
> > The AC_APPEND_FLAG doesn't provide the functionality I would want for 
> > portability,
> > because it doesn't really check if the flag works (and it doesn't report 
> > the checking either)....
> > Anyway, I think you're on the right path, just need some tweaks before it's 
> > ready.


On Mon, 10 Feb 2014 14:49:30 -0500, Dale Visser <address@hidden> wrote:
> Your point is well-taken. Please find attached a revision of the earlier 
> patch. This one includes AC_APPEND_*FLAG_IFVALID macros (* = C, CXX or FC) 
> that will append flags to the compiler invocation only if a test compilation 
> with those flags exits normally. In addition, I've made clarifications in the 
> documentation about how to not add the newly default warning flags if 
> desired, and I've standardized the informative printed output generated by 
> the macros when configure is run.

I like new the informative printed output. That really helps and is consistent
with what other autoconf probing macros do.  For example, I now see:
> checking CFLAGS for maximum warnings... -Wall

However, the "IFVALID" macro doesn't create informative printed output.
It really should produce a message that it's probing, and what the results are.
When things go wrong that info can really help.  Could you add that printed 
output
for IFVALID, so that it prints that it *is* probing, and once done prints the 
result?

One *weird* thing: the _IFVALID macros, when sent to gcc, will accept
warning flags that don't actually *do* anything.  That's because gcc doesn't
complain about invalid warning flags beginning with "-W".
For example, if you add this:
  AC_APPEND_FLAG_IFVALID([-Wno-such-option])
the compilation will succeed (error return 0) without any commentary about the
unimplemented -Wno-such-option, and thus, the flag *will* be added.
That's odd to my way of thinking, but it's completely consistent with your
documentation, and on reflection I think perhaps that's okay.
After all, if the flag is added but does no harm (e.g., the compilation will 
succeed),
it makes sense to go ahead and add it.  This suggests that you need to 
carefully word
the "IFVALID" macro output, something like this:
  checking if -Wno-such-option can be added to CFLAGS... yes
so that users know that it "can be added", vs. "is supported".
So I guess no proposed change here, just be careful on the wording.

I was going to complain that you only supported language-specific macros, and
that you should support a general "AC_APPEND_FLAG_IFVALID".  But you *did* 
include one,
so I guess that complaint has already been shot down :-).

I did find a weirdness.  When rebuilding this patched autoconf, it rebuilds the 
documentation,
and that rebuild is generating warnings.  That suggests a minor problem in the 
documentation.
Here are the warnings:
./doc/autoconf.texi:23468: warning: `(' follows defined name 
`AC_CFLAGS_WARN_ALL' instead of whitespace.
./doc/autoconf.texi:23480: warning: `(' follows defined name 
`AC_CXXFLAGS_WARN_ALL' instead of whitespace.
./doc/autoconf.texi:23492: warning: `(' follows defined name 
`AC_FCFLAGS_WARN_ALL' instead of whitespace.
./doc/autoconf.texi:23504: warning: `(' follows defined name 
`AC_FLAGS_WARN_ALL' instead of whitespace.
./doc/autoconf.texi:23526: warning: `(' follows defined name `AC_APPEND_FLAG' 
instead of whitespace.
./doc/autoconf.texi:23534: warning: `(' follows defined name 
`AC_APPEND_CFLAG_IFVALID' instead of whitespace.
./doc/autoconf.texi:23549: warning: `(' follows defined name 
`AC_APPEND_CXXFLAG_IFVALID' instead of whitespace.
./doc/autoconf.texi:23564: warning: `(' follows defined name 
`AC_APPEND_FCFLAG_IFVALID' instead of whitespace.
./doc/autoconf.texi:23579: warning: `(' follows defined name 
`AC_APPEND_FLAG_IFVALID' instead of whitespace.

Can you fix the documentation to remove those warnings?

Also, I have a a few other minor comments about the documentation text.
I think the text should FIRST say what it does, then later who uses it.
It should also cross-reference other related macros if the user might want to 
use that one instead,
in particular, I think AC_APPEND_FLAG should cross-reference to 
AC_APPEND_FLAG_IFVALID,
because I think most users would typically use that instead (or at least want 
to know about it).

So in particular given:
@anchor{AC_APPEND_FLAG}
@defmac AC_APPEND_FLAG(@var{add-value})
@acindex{APPEND_FLAG}
Used internally by @pxref{AC_FLAGS_WARN_ALL}. Behavior is to append the given to
the shell variable determined or provided in the call to
@pxref{AC_FLAGS_WARN_ALL}, but only if it is not already present there.
@end defmac

I think the main text for AC_APPEND_FLAG should be something like:
Append text @var{add-value}, if it's not already present, to the flag variable 
given
(if not given the current flag variable is used, e.g., CFLAGS or CXXFLAGS).
This is used internally by @pxref{AC_FLAGS_WARN_ALL}.
You may wish to use @pxref{AC_APPEND_FLAG_IFVALID} instead.

> I think this change to the default behavior of AC_PROG_* (* =  CC, CXX or FC) 
> will help many projects using autoconf to become aware of issues in their 
> code, ultimately helping to make it more reliable and secure. And as noted in 
> the documentation changes, it is straightforward to restore the old, 
> non-flag-adding behavior, as well as to add compiler flags you find useful 
> for your particular code base.

Agree!

--- David A. Wheeler