Improve Autoconf C compiler feature detection

[MisterDA]

This PR:

• switches from custom macros testing support of compiler flags to using the autoconf-archive AX_CHECK_COMPILE_FLAG macro, and updates it;
• fixes compilers failing to build C snippets if they reject non strict prototypes by default.

no change entry needed cc @shindere

[shindere]

I very much like this PR, thanks a lot!

I have a few questions / suggestions:

First commit: I suggest to rewrite the commit message to:

Use Autoconf's AX_CHECK_COMPILE_FLAG macro consistently

Also, since we are passing [$warn_error_flag] systematically, I am wondering whether it wouldn't be worth introducing our own OCAML_CHECK_COMPILE_FLAG which would basically not do much more than calling the original AX_CHECK_COMPILE_FLAG with the 3 arguments it is given, plus the [$warn_error_flag] one. What do you think, @MisterDA?

One other observation is that, with the new code, most of the time we write the flags twice: once to test whether it's supported, once to add it to a variable. I think this is something we were not doing before and I slightly regret we have to do it now. I was wondering whether that's something we could avoid by making the OCAML_CHECK_COMPILE_FLAG macro mentionned above a bit less generic. I'm not 10% sure it's doable but still wanted to share the thought.

Regarding your second commit, Make aclocal.m4 macros -Wstrict-prototypes compatible, wouldn't it be worth introducing a C_MINIMAL_PROGRAM macro to avoid repeating it?

You could even introduce the macro in one commit and then fix it in another one. ;)

[MisterDA]

First commit: I suggest to rewrite the commit message to:

Use Autoconf's AX_CHECK_COMPILE_FLAG macro consistently

Ok.

Also, since we are passing [$warn_error_flag] systematically, I am wondering whether it wouldn't be worth introducing our own OCAML_CHECK_COMPILE_FLAG which would basically not do much more than calling the original AX_CHECK_COMPILE_FLAG with the 3 arguments it is given, plus the [$warn_error_flag] one. What do you think, @MisterDA?

This is a nice idea, I'll try it out. I agree that having to pass [$warn_error_flag] every time is unfortunate. I'm tempted not to introduce a new abstraction and keep the code a bit more verbose, but explicit.

One other observation is that, with the new code, most of the time we write the flags twice: once to test whether it's supported, once to add it to a variable. I think this is something we were not doing before and I slightly regret we have to do it now. I was wondering whether that's something we could avoid by making the OCAML_CHECK_COMPILE_FLAG macro mentioned above a bit less generic. I'm not 10% sure it's doable but still wanted to share the thought.

We were implicitly doing this already: for instance for the -fno-tree-vrp flag, it was copied in AC_MSG_CHECKING, in the compiler invocation, and when adding it to the internal_cflags. Switching to AX_CHECK_COMPILE_FLAG actually removes one occurrence.

Regarding your second commit, Make aclocal.m4 macros -Wstrict-prototypes compatible, wouldn't it be worth introducing a C_MINIMAL_PROGRAM macro to avoid repeating it? You could even introduce the macro in one commit and then fix it in another one. ;)

Good idea, and that's Autoconf AC_LANG_PROGRAM.

[MisterDA]

I've given OCAML_CHECK_COMPILE_FLAG some thoughts, and my conclusion is that if we just want to avoid passing [$warn_error_flag], then it boils down to:

AC_DEFUN([OCAML_CHECK_COMPILE_FLAG], [
  AX_CHECK_COMPILE_FLAG([$1], [$2], [$3], [${4:-$warn_error_flag}], [$5])])

and I'm not sure that it's a tremendous win.

I couldn't find a nice way of factoring the macros to avoid repeating the flag, because we have slightly different use cases. I was leaning towards something like below, which I think would have supported all our use-cases:

dnl $1: a flag to test
dnl $2: a variable to append $1 to if supported, or a shell command executed if the flag is supported
AC_DEFUN([OCAML_CHECK_COMPILE_FLAG], [
  AS_IF([is_valid_variable_name "$2"],
    [success="\"$2=\$$2\${$2:+ }$1\""],
    [success="$2"])
  AX_CHECK_COMPILE_FLAG(
    [$1],
    [eval "$success"],
    [$3],
    [${4:-$warn_error_flag}],
    [$5])])

but in practice it seems really hard to implement in posix shell.

In the end, I'm inclined to keep the verbose but explicit calls to AX_COMPILE_FLAGS. Let me know if you have a better idea, I'm all ears!

I've also switched to using AC_LANG_PROGRAM everywhere.

[MisterDA]

AS_IF([is_valid_variable_name "$2"],

could be written as

AS_IF([echo "$2" | grep -q '^[_[:alpha:]][_[:alpha:][:digit:]]*$'],

is this something we'd really be interested in using?

[shindere]

Antonin Décimo (2024/04/19 14:11 -0700):

In the end, I'm inclined to keep the verbose but explicit calls to AX_COMPILE_FLAGS. Let me know if you have a better idea, I'm all ears!

I don't, at the moment, so let's go with the current implementation.

Many thanks for having taken the time to investigate anyway.

I've also switched to using AC_LANG_PROGRAM everywhere.

I just reviewed this commit. For those who didn't read the documentation, the macro takes 2 arguemnts: the first is the prelude of the programs (everything appearing before the main function), and the second argument is main's body, given that autconf will add a final return 0;. It is important to keep this detail in mind while reviewing this commit, otherwiuse the replacement of "return expr; statement by if (expr) return 1; looks slightly odd (at least, did look slightly odd to me). The change made me grumpy, I was going to suggest to add a comment where approrpiate to clarify that a return 0; statement is added, but then I didn't dare to moan because I was afraid of being seen as the «vieux con» I may start to actually be.