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

From: Kyle Lippincott <spectral@google.com>
Date: Feb 27, 2024, 20:10 UTC
Message-ID: <CAO_smVg9phJGv=fMcHXjqvY74pHuMdousqfLMpyBcQ+fVMY90g@mail.gmail.com>
In-Reply-To: <20240227084529.GJ3263678@coredump.intra.peff.net>

On Tue, Feb 27, 2024 at 12:45 AM Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

>
> 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.

Show 25 quoted lines

>
> 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 :)

Show 7 quoted lines

>
> 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