Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2020, 17:30 UTC
Message-ID: <xmqqtutx6rwc.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqy2j96sd4.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 46 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> As a maintainer, I am less concerned about the "result today" than I am
>> about keeping things easy and effortless to maintain. One of your patches
>> accomplishes that. The other one made it into `next`:
>> https://github.com/git/git/commit/91a67b86f77
>
> I do not think reverting it and requeuing
>
> https://lore.kernel.org/git/20201107221916.1428757-1-dgurney99@gmail.com/
>
> would help future folks why we ignore _MSC_VER as any sign usable to
> detect endianness, so I'd prefer to see a patch *on top* of 1af265f0
> (compat/bswap.h: simplify MSVC endianness detection, 2020-11-08),
> which is 91a67b86f77^2, that explains why we prefer to list archs
> explicitly in its log message, which would be the primary value of
> that commit.
>
> Something along this line, perhaps?
>
> -- >8 --
>
> Subject: compat/bswap.h: do not assume MSVC is little-endian only
>
> Earlier, with 1af265f0 (compat/bswap.h: simplify MSVC endianness
> detection, 2020-11-08), we tried to simplify endianness detection
> used in compat/bswap.h by assuming that any version Git compiled by
> MSVC (detected by _MSC_VER preprocessor macro) is meant to run on
> little endian boxes, as the versions of old MSVC that support m68k
> and MIPS do not support some C99 features used in the codebase
> anyway.
>
> While it might hold true that modern versions of Windows are all
> little-endian, MSVC is and/or can be ported to build for big-endian
> boxes, so tying _MSC_VER with endianness is a bit too restrictive.
>
> Let's go back to the old way to use _MSC_VER to learn what
> preprocessor macros compiler uses to tell us which arch we are
> building for, and list these arches that are little-endian
> explicitly.
>
> ... signed-off-by from you and helped-by from others ...
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>     diffstat
>     patch

Daniel's patch adds _M_ARM64 to the list, but do we need to do anything further to tell the endian on such a bi-endian arch, or does MSVC only support little-endian for that architecture?

Just double-checking as the "confusion" that started this thread came from an assumption that MSVC == Windows == big-endian, and you told us MSVC != Windows. Now the patch assumes ARM64-on-MSVC is little-endian only and we want to make sure that assumption is true.

And perhaps it is worth documenting in the log, perhaps

	... that are little-endian explicitly.  Note that ARM64 is
	bi-endian in nature but we treat it little-endian as MSVC
	does not treat the arch as bi-endian.

or something like that at the end (I do not know what MSVC actually does---just illustrating the level of details I expect in the explanation).

Thanks.