Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used

From: Junio C Hamano <gitster@pobox.com>
To: Kyle Lippincott <spectral@google.com>
Cc: Calvin Wan <calvinwan@google.com>,
	 git@vger.kernel.org,  Jonathan Tan <jonathantanmy@google.com>,
	 phillip.wood123@gmail.com
Subject: Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
Date: Mon, 26 Feb 2024 18:45:43 -0800	[thread overview]
Message-ID: <xmqqzfvmhbfs.fsf@gitster.g> (raw)
In-Reply-To: <CAO_smVjeNYFTgh4MZjRM9U1coY=UJxo-E6bD9OfdS1A1Xf6vcQ@mail.gmail.com> (Kyle Lippincott's message of "Mon, 26 Feb 2024 16:56:28 -0800")

Kyle Lippincott <spectral@google.com> writes:

>> In any case, your sources should not include a standard library
>> header directly yourself, period.  Instead let <git-compat-util.h>
>> take care of the details of how we need to obtain what we need out
>> of the system on various platforms.
>
> I disagree with this statement. We _can't_ use a magic compatibility
> header file in the library interfaces, for the reasons I outlined
> further below in my previous message. For those headers, the ones that
> might be included by code that's not under the Git project's control,
> they need to be self-contained, minimal, and maximally compatible.

Note that I am not talking about your random outside program that
happens to link with gitstdlib.a; it would want to include a header
file <gitstdlib.h> that comes with the library.

Earlier I suggested that you may want to take a subset of
<git-compat-util.h>, because <git-compat-util.h> may have a lot more
than what is minimally necessary to allow our sources to be
insulated from details of platform dependence.  You can think of
that subset as a good starting point to build the <gitstdlib.h>
header file to be given to the library customers.

But the sources that go to the library, as gitstdlib.a is supposed
to serve as a subset of gitlib.a to our internal codebase when
building the git binary, should still follow our header inclusion
rules.

Because we would want to make sure that the sources that are made
into gitstdlib.a, the sources to the rest of libgit.a, and the
sources to the rest of git, all agree on what system features we ask
from the system, feature macros that must be defined to certain
values before we include system library files (like _XOPEN_SOURCE
and _FILE_OFFSET_BITS) must be defined consistently across all of
these three pieces.  One way to do so may be to ensure that the
definition of them would be migrated to <gitstdlib.h> when we
separate a subset out of <git-compat-util.h> to it (and of course,
we make <git-compat-util.h> to include <gitstdlib.h> so that it
would be still sufficient for our in-tree users to include the
<git-compat-util.h>)

<gitstdlib.h> may have to expose an API function that uses some
extended types only available by including system header files,
e.g. some function may return ssize_t as its value or take an off_t
value as its argument.

If our header should include system headers to make these types
available to our definitions is probably open to discussion.  It is
harder to do so portably, unless your world is limited to POSIX.1
and ISO C, than making it the responsibility of library users.  

But if the platform headers and libraries support feature macros
that allows you to tweak these sizes (e.g. the size of off_t may be
controlled by setting the _FILE_OFFSET_BITS to an appropriate
value), it may be irresponsible to leave that to the library users,
as they MUST make sure to define such feature macros exactly the
same way as we define for our code, which currently is done in
<git-compat-util.h>, before they include their system headers to
obtain off_t so that they can use <gitstdlib.h>.

So the rules for library clients (random outside programs that
happen to link with gitstdlib.a) may not be that they must include
<git-compat-util.h> as the first thing, but they probably still have
to include <gitstdlib.h> fairly early before including any of their
system headers, I would suspect, unless they are willing to accept
such responsibility fully to ensure they compile the same way as the
gitstdlib library, I would think.