Hi Junio, On Tue, 10 Nov 2020, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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@xxxxxxxxx/ > > > > 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@xxxxxxxxx> > > --- > > 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? Yes, MSVC only supports little-endian for that architecture. From https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-160#endianness: Endianness As with the ARM32 version of Windows, on ARM64 Windows executes in little-endian mode. Switching endianness is difficult to achieve without kernel mode support in AArch64, so it's easier to enforce. > 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. I did not say that MSVC != Windows, but Visual Studio != Windows. But I did say that I do not want to assume MSVC == Windows for all eternity. > 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). Absolutely. If nobody beats me to the punch, I hope to get around to prepare the patch that you suggested. Thanks, Dscho
