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

[Jeff King <peff@peff.net>]

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

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