On Mon, Feb 26, 2024 at 04:56:28PM -0800, Kyle Lippincott wrote:

quoted

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