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

[Kyle Lippincott <spectral@google.com>]

On Tue, Feb 27, 2024 at 12:45 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Feb 26, 2024 at 04:56:28PM -0800, Kyle Lippincott wrote:
>
> > > We use inttypes.h by default because the C standard already talks
> > > about it, and fall back to stdint.h when the platform lacks it.  But
> > > what I suspect is that nobody compiles with NO_INTTYPES_H and we
> > > would unknowingly (but not "unintentionally") start using the
> > > extended types that are only available in <inttypes.h> but not in
> > > <stdint.h> sometime in the future.  It might already have happened,
> >
> > It has. We use PRIuMAX, which is from inttypes.h.
>
> Is it always, though? That's what C99 says, but if you have a system
> that does not have inttypes.h in the first place, but does have
> stdint.h, it seems possible that it provides conversion macros elsewhere
> (either via stdint.h, or possibly just as part of stdio.h).

It's of course possible that on some platforms, stdio.h or stdint.h
defines these types (or includes inttypes.h internally, which defines
these types). However, I think that to be "correct" and for a compiler
to claim it supports C99 (and the compiler _must_ claim that because
of the guard in <git-compat-util.h>), inttypes.h must exist, and it
must cause these symbols to appear. If there are platforms that are
claiming to be C99 and inttypes.h doesn't exist or doesn't provide the
symbols it should, I don't think we should try to support them - they
can maintain platform-specific patches for whatever not-actually-C99
language the platform supports. Basically what git for windows is
already doing (presumably for other reasons), as far as I can tell.

>
> So it might be that things have been horribly broken on NO_INTTYPES_H
> systems for a while, and nobody is checking. But I don't think you can
> really say so without looking at such a system.
>
> And looking at config.mak.uname, it looks like Windows is such a system.
> Does it really have inttypes.h and it is getting included from somewhere
> else, making format conversion macros work? Or does it provide those
> macros elsewhere, and really needs stdint? It does look like
> compat/mingw.h includes it, but I think we wouldn't use that for msvc
> builds.
>
> > I think it's only
> > "accidentally" working if anyone uses NO_INTTYPES_H. I changed my
> > stance halfway through this investigation in my previous email, I
> > apologize for not going back and editing it to make it clear at the
> > beginning that I'd done so. My current stance is that
> > <git-compat-util.h> should be either (a) including only inttypes.h
> > (since it includes stdint.h), or (b) including both inttypes.h and
> > stdint.h (in either order), just to demonstrate that we can.
>
> It is good to clean up old conditionals if they are no longer
> applicable, as they are a burden to reason about later (as this
> discussion shows). But I am not sure about your "just to demonstrate we
> can".

Yeah, I'm also not convinced the "just to demonstrate we can" has much
value. I was trying to get ahead of future discussions where we claim
it's important to never include stdint.h (because people remember this
discussion and how contentious it was) and think it might misbehave,
and instead just include it in <git-compat-util.h> to prove it
_doesn't_ misbehave, and thus start to allow usage in self-contained
headers when necessary.

> It is good to try it out, but it looks like there is a non-zero
> chance that MSVC on Windows might break. It is probably better to try
> building there or looping in folks who can, rather than just making a
> change and seeing if anybody screams.

I think I miscommunicated here, or had too many assumptions about the
current state of things that I didn't actually verify. When I wrote
"and seeing if the build bots or maintainers identify it as a
continuing issue", I was assuming that we had build bots for all major
platforms (including windows, with however it gets built: mingw or VC
or whatever), and I included the "maintainers" part of it for the long
tail of esoteric platforms that we either don't know about, or can't
have automated builds on for whatever reason. I agree that making
changes that have a high likelihood of breaking supported platforms
(which gets back to that platform support thread that was started a
few weeks ago) should not be done lightly, and it's not reasonable to
make the change and wait for maintainers of these "supported
platforms" to complain. I was relying on the build bots covering the
"supported platforms" and stopping me from even sending such a patch
to the mailing list :)

>
> I think the "win+VS" test in the GitHub Actions CI job might cover this
> case. It is not run by default (because it was considered be mostly
> redundant with the mingw build), but it shouldn't be too hard to enable
> it for a one-off test.
>
> -Peff