From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 12 Jun 2026 13:27:36 GMT
Subject: Re: [PATCH v3 2/2] compat/posix.h: simplify GIT_GNUC_PREREQ() comparison
Message-ID: <aiwJSBfRbUFZ70gP@pks.im>
In-Reply-To: <20260608124419.38905-2-dominik.loidolt@univie.ac.at>

On Mon, Jun 08, 2026 at 02:44:19PM +0200, Dominik Loidolt wrote:
> Replace the glibc-style bit-shift version comparison with an explicit
> major/minor comparison. This is easier to read and is consistent with
> the format already used by GIT_CLANG_PREREQ() and many BSD

It's a bit funny to use `GIT_CLANG_PREREQ()` as an argument here as
we've just added it in the preceding commit.

> <sys/cdefs.h> headers.
> 
> This has no runtime impact, as the macro is evaluated at compile time.
> It is also more future-proof, as it no longer assumes that GCC version
> components stay below 65536.

I feel like all the message needs to say is "let's do it for
consistency, and it's easier to read". That would've been sufficient,
whereas this argument here feels a bit thin.

Doesn't matter much though, and I think ultimately the message is fine
as-is, even though the reasoning is a bit funny.

> diff --git a/compat/posix.h b/compat/posix.h
> index ffdfd91c7b..deefc43f28 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -4,22 +4,24 @@
>  #define _FILE_OFFSET_BITS 64
>  
>  /*
> - * Derived from Linux "Features Test Macro" header
> - * Convenience macros to test the versions of gcc (or
> - * a compatible compiler).
> + * Convenience macros to test the versions of GCC (or a compatible compiler).
>   * Use them like this:
>   *  #if GIT_GNUC_PREREQ (2,8)
> - *   ... code requiring gcc 2.8 or later ...
> + *   ... code requiring GCC 2.8 or later ...
>   *  #endif
>   *
> + * Note that Clang and other compilers define __GNUC__ for compatibility; use
> + * GIT_CLANG_PREREQ() to check for specific Clang versions.
> + *
>   * This macro of course is not part of POSIX, but we need it for the UNUSED
>   * macro which is used by some of our POSIX compatibility wrappers.
> -*/
> + */

It would've been nice to either move these changes into a preparatory
commit or at least mention them 

>  #if defined(__GNUC__) && defined(__GNUC_MINOR__)
>  # define GIT_GNUC_PREREQ(maj, min) \
> -	((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min))
> +	((__GNUC__ > (maj)) || \
> +	 (__GNUC__ == (maj) && __GNUC_MINOR__ >= (min)))
>  #else
> - #define GIT_GNUC_PREREQ(maj, min) 0
> +# define GIT_GNUC_PREREQ(maj, min) 0
>  #endif

The change itself makes sense to me.

I'm not sure myself whether this could use another reroll. It's all just
nits, and the intent is clear enough.

Thanks!

Patrick