On Fri, Mar 14, 2025 at 03:29:54PM -0700, Junio C Hamano wrote:
> ---- >8 ----
> Our hope is that the number of code paths that falsely trigger
> warnings with the -Wunreachable-code compilation option are small,
> and they can be worked around case-by-case basis, like we just did
> in the previous commit. If we need such a workaround a bit more
> often, however, we may benefit from a more generic and descriptive
> facility that helps document the cases we need such workarounds.
>
> Side note: if we need the workaround all over the place, it
> simply means -Wunreachable-code is not a good tool for us to
> save engineering effort to catch mistakes. We are still
> exploring if it helps us, so let's assume that it is not the
> case.
Yup, I very much agree with this, especially the side note. (I'd
probably have just dropped patch 2 and gone straight here, but I don't
mind leaving it in as documentation of that other direction).
> Introduce NOT_A_CONST() macro, with which, the developer can tell
> the compiler:
>
> Do not optimize this expression out, because, despite whatever
> you are told by the system headers, this expression should *not*
> be treated as a constant.
This is definitely better than the other name. I might spell it out
as "NOT_A_CONSTANT", just because "const" to me is a variable annotation
(for something that _could_ change, but we are not allowed to). Whereas
"constant" is something defined to a single value in the program. Maybe
splitting hairs, but as somebody who read NOT_A_CONST(foo) I might
expect it to be casting away "const" or something.
> --- a/Makefile
> +++ b/Makefile
> @@ -1018,6 +1018,7 @@ LIB_OBJS += ewah/ewah_bitmap.o
> LIB_OBJS += ewah/ewah_io.o
> LIB_OBJS += ewah/ewah_rlw.o
> LIB_OBJS += exec-cmd.o
> +LIB_OBJS += fbtcdnki.o
That name is a mouthful, for sure. The long name is really an
implementation detail. Would calling it not-constant.c or something be
more descriptive? (Yes, the macro itself does not appear in the file,
but hopefully it links the two semantically in the reader's head).
I almost want to suggest a name like "compiler-tricks.c", but part of
the point of this particular trick is that there's nothing else in its
translation unit. So later when somebody adds another trick, it cannot
use this macro. ;)
> +/*
> + * Prevent an overly clever compiler from optimizing an expression
> + * out, triggering a false positive when building with the
> + * -Wunreachable-code option. false_but_the_compiler_does_not_know_it_
> + * is defined in a compilation unit separate from where the macro is
> + * used, initialized to 0, and never modified.
> + */
> +#define NOT_A_CONST(expr) ((expr) || false_but_the_compiler_does_not_know_it_)
> +extern int false_but_the_compiler_does_not_know_it_;
Good explanation. I do wonder if we'd eventually see a compiler that
reaches across translation units to optimize, but I'd hope we probably
bought ourselves a decade or two.
> diff --git a/run-command.c b/run-command.c
> index d527c46175..535c73a059 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -516,14 +516,12 @@ static void atfork_prepare(struct atfork_state *as)
> sigset_t all;
>
> /*
> - * Do not use the return value of sigfillset(). It is transparently 0
> - * on some platforms, meaning a clever compiler may complain that
> - * the conditional body is dead code. Instead, check for error via
> - * errno, which outsmarts the compiler.
> + * POSIX says sitfillset() can fail, but an overly clever
> + * compiler can see through the header files and decide
> + * it cannot fail on a particular platform it is compiling for,
> + * triggering -Wunreachable-code false positive.
> */
> - errno = 0;
> - sigfillset(&all);
> - if (errno)
> + if (NOT_A_CONST(sigfillset(&all)))
> die_errno("sigfillset");
And this looks much nicer and more descriptive. You could probably even
get away without the comment, but I certainly do not mind it.
s/sitfillset/sigfillset/ in your comment text, though.
-Peff
