Re: [PATCH v3 2/3] git-compat-util: add NOT_CONSTANT macro and use it in atfork_prepare()
- From
Calvin Wan <calvinwan@google.com>
- Date
- Mar 18, 2025, 22:04 UTC
- Message-ID
- <20250318220453.1937685-1-calvinwan@google.com>
- In-Reply-To
- <20250317235329.809302-3-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 64 quoted lines
> 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.
>
> Introduce NOT_CONSTANT() 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.
>
> and use it as a replacement for the workaround we used that was
> somewhat specific to the sigfillset case. If the compiler already
> knows that the call to sigfillset() cannot fail on a particular
> platform it is compiling for and declares that the if() condition
> would not hold, it is plausible that the next version of the
> compiler may learn that sigfillset() that never fails would not
> touch errno and decide that in this sequence:
>
> errno = 0;
> sigfillset(&all)
> if (errno)
> die_errno("sigfillset");
>
> the if() statement will never trigger. Marking that the value
> returned by sigfillset() cannot be a constant would document our
> intention better and would not break with such a new version of
> compiler that is even more "clever". With the marco, the above
> sequence can be rewritten:
>
> if (NOT_CONSTANT(sigfillset(&all)))
> die_errno("sigfillset");
>
> which looks almost like other innocuous annotations we have,
> e.g. UNUSED.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Makefile | 1 +
> compiler-tricks/not-a-constant.c | 2 ++
> git-compat-util.h | 9 +++++++++
> meson.build | 1 +
> run-command.c | 12 +++++-------
> 5 files changed, 18 insertions(+), 7 deletions(-)
> create mode 100644 compiler-tricks/not-a-constant.c
>
> diff --git a/Makefile b/Makefile
> index 97e8385b66..605e2d7f61 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -985,6 +985,7 @@ LIB_OBJS += compat/nonblock.o
> LIB_OBJS += compat/obstack.o
> LIB_OBJS += compat/terminal.o
> LIB_OBJS += compat/zlib-uncompress2.o
> +LIB_OBJS += compiler-tricks/not-a-constant.oThe name is correctly added here, but in `next,` this name is set to `compiler-tricks/not-constant.o`.