Skip to contentSkip to conversation

git-compat-util.h: drop the PRIuMAX definition

PATCH In review Hariom Verma via GitGitGadget 4 participants 8 messages Nov 24, 2019 – Nov 25, 2019

lore
Share & tools
lore
#0 lore raw

cover git-compat-util.h: drop the PRIuMAX definition

Git's code base already seems to be using PRIdMAX without any such fallback definition for quite a while (75459410edd (json_writer: new routines to create JSON data, 2018-07-13), to be precise, and the first Git version to include that commit was v2.19.0).

Therefore it should be safe to drop the fallback definition for PRIuMAX in git-compat-util.h.

This addresses https://github.com/gitgitgadget/git/issues/399

Hariom Verma (1):
git-compat-util.h: drop the PRIuMAX definition

 git-compat-util.h | 4 ----
 1 file changed, 4 deletions(-)

base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-473%2Fharry-hov%2Fpriumax-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-473/harry-hov/priumax-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/473

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/1 git-compat-util.h: drop the `PRIuMAX` definition

From: Hariom Verma <[email protected]>

Git's code base already seems to be using PRIdMAX without any such fallback definition for quite a while (75459410edd (json_writer: new routines to create JSON data, 2018-07-13), to be precise, and the first Git version to include that commit was v2.19.0).

Therefore it should be safe to drop the fallback definition for PRIuMAX in git-compat-util.h.

This addresses https://github.com/gitgitgadget/git/issues/399

---
 git-compat-util.h | 4 ----
 1 file changed, 4 deletions(-)
git-compat-util.h +0 -4
index 607dca7534..ba710cfa6c 100644
@@ -320,10 +320,6 @@ char *gitdirname(char *);
 #define PATH_MAX 4096
 #endif
 
-#ifndef PRIuMAX
-#define PRIuMAX "llu"
-#endif
-
 #ifndef SCNuMAX
 #define SCNuMAX PRIuMAX
 #endif
Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/1] git-compat-util.h: drop the `PRIuMAX` definition

Quoted 9 lines · Hariom Verma via GitGitGadget

From: Hariom Verma <[email protected]>

Git's code base already seems to be using PRIdMAX without any such fallback definition for quite a while (75459410edd (json_writer: new routines to create JSON data, 2018-07-13), to be precise, and the first Git version to include that commit was v2.19.0).

Therefore it should be safe to drop the fallback definition for PRIuMAX in git-compat-util.h.

I noticed this recently, too, and wondered if it was time for a cleanup.

We do sometimes get portability reports more than a year after the problem was introduced. But I think this one is pretty safe. PRIuMAX is in C99, and we've been picking up other C99-isms without complaint.

I was curious what system originally spurred this. The PRIuMAX definition was originally added in 3efb1f343a (Check for PRIuMAX rather than NO_C99_FORMAT in fast-import.c., 2007-02-20). But it was replacing a construct that was introduced in 579d1fbfaf (Add NO_C99_FORMAT to support older compilers., 2006-07-30), which talks about gcc 2.95. That's pretty ancient at this point.

git-compat-util.h char *gitdirname(char *); · line 326

Quoted 12 lines
git-compat-util.h +0 -4
index 607dca7534..ba710cfa6c 100644
@@ -320,10 +320,6 @@ char *gitdirname(char *);
 #define PATH_MAX 4096
 #endif
 
-#ifndef PRIuMAX
-#define PRIuMAX "llu"
-#endif
-

This part of the patch looks obviously correct. :) But...

git-compat-util.h char *gitdirname(char *); · line 325

Quoted 3 lines
git-compat-util.h +0 -0
 #ifndef SCNuMAX
 #define SCNuMAX PRIuMAX
 #endif

Can we likewise ditch the fallback definition for SCNuMAX? And PRIu32, etc? It seems likely any platform would either have all of them or none.

-Peff

#3 lore raw

Re: [PATCH 1/1] git-compat-util.h: drop the `PRIuMAX` definition

Quoted 4 lines · Jeff King

We do sometimes get portability reports more than a year after the problem was introduced. But I think this one is pretty safe. PRIuMAX is in C99, and we've been picking up other C99-isms without complaint.

I think the problem might come from places where the default compiler is still not C99 by default (ex: old CentOS) and any other places using gcc < 5 which is less ancient

Carlo

#4 lore raw

Re: [PATCH 1/1] git-compat-util.h: drop the `PRIuMAX` definition

Quoted 3 lines · Carlo Arenas

I think the problem might come from places where the default compiler is still not C99 by default (ex: old CentOS)

CentOS 6.10 (using gcc 4.4.7) compiles this without problems by default so nothing to worry about, sorry for the red herring

Carlo

PS. non GNU89 (AKA ANSI) mode will obviously break but not ONLY because of this change

#5 lore raw

Re: [PATCH 1/1] git-compat-util.h: drop the `PRIuMAX` definition

In reply to Jeff King
Quoted 9 lines · Jeff King
Quoted 7 lines · Hariom Verma via GitGitGadget

From: Hariom Verma <[email protected]>

Git's code base already seems to be using PRIdMAX without any such fallback definition for quite a while (75459410edd (json_writer: new routines to create JSON data, 2018-07-13), to be precise, and the first Git version to include that commit was v2.19.0).

Quoted 4 lines
Quoted 2 lines

Therefore it should be safe to drop the fallback definition for PRIuMAX in git-compat-util.h.

I noticed this recently, too, and wondered if it was time for a cleanup.

While I agree with the conclusion, I do not think I agree with the above "Therefore (implying that the lack of need for fallback PRIdMAX means the same for PRIuMAX) it should be safe" as a good justification. That reasoning assumes that the outside world is much saner than us. We thought PRIuMAX fallback necessary while a counterpart for PRIdMAX unneeded---the outside world could have made a similar mistake and in the opposite way (i.e. only defined PRIdMAX while leaving PRIuMAX undefined).

But I do agree with the alternative justification in the following two paragraphs you have given, which are ...

Quoted 10 lines

We do sometimes get portability reports more than a year after the problem was introduced. But I think this one is pretty safe. PRIuMAX is in C99, and we've been picking up other C99-isms without complaint.

I was curious what system originally spurred this. The PRIuMAX definition was originally added in 3efb1f343a (Check for PRIuMAX rather than NO_C99_FORMAT in fast-import.c., 2007-02-20). But it was replacing a construct that was introduced in 579d1fbfaf (Add NO_C99_FORMAT to support older compilers., 2006-07-30), which talks about gcc 2.95. That's pretty ancient at this point.

... these.

Quoted 8 lines

This part of the patch looks obviously correct. :) But...

Quoted 3 lines

#ifndef SCNuMAX
#define SCNuMAX PRIuMAX
#endif

Can we likewise ditch the fallback definition for SCNuMAX? And PRIu32, etc? It seems likely any platform would either have all of them or none.

I guess that's also a C99-ism that we can use?

Thanks, both.

#6 lore raw

Re: [PATCH 1/1] git-compat-util.h: drop the `PRIuMAX` definition

Quoted 6 lines · Junio C Hamano
Quoted 2 lines

Can we likewise ditch the fallback definition for SCNuMAX? And PRIu32, etc? It seems likely any platform would either have all of them or none.

I guess that's also a C99-ism that we can use?

Thanks, both.

Here is what I have locally for now.

1: 98f866a929 ! 1: ebc3278665 git-compat-util.h: drop the PRIuMAX definition
@@ Metadata
Author: Hariom Verma <[email protected]>

## Commit message ##
- git-compat-util.h: drop the PRIuMAX definition
+ git-compat-util.h: drop the PRIuMAX and other fallback definitions

Git's code base already seems to be using PRIdMAX without any such fallback definition for quite a while (75459410edd (json_writer: new routines to create JSON data, 2018-07-13), to be precise, and the
- first Git version to include that commit was v2.19.0).
+ first Git version to include that commit was v2.19.0). Having a
+ fallback definition only for PRIuMAX is a bit inconsistent.

- Therefore it should be safe to drop the fallback definition for
- PRIuMAX in git-compat-util.h.
+ We do sometimes get portability reports more than a year after the
+ problem was introduced. This one should be fairly safe. PRIuMAX is
+ in C99 (for that matter, SCNuMAX, PRIu32 and others also are), and
+ we've been picking up other C99-isms without complaint.

- This addresses https://github.com/gitgitgadget/git/issues/399
+ The PRIuMAX fallback definition was originally added in 3efb1f343a
+ (Check for PRIuMAX rather than NO_C99_FORMAT in fast-import.c.,
+ 2007-02-20). But it was replacing a construct that was introduced in
+ an even earlier commit, 579d1fbfaf (Add NO_C99_FORMAT to support
+ older compilers., 2006-07-30), which talks about gcc 2.95.
+
+ That's pretty ancient at this point.

+ Helped-by: Jeff King <[email protected]>
+ [jc: tweaked both message and code, taking what peff wrote]

## git-compat-util.h ##
@@ git-compat-util.h: char *gitdirname(char *);
-#define PRIuMAX "llu"
-#endif
-
- #ifndef SCNuMAX
- #define SCNuMAX PRIuMAX
- #endif
+-#ifndef SCNuMAX
+-#define SCNuMAX PRIuMAX
+-#endif
+-
+-#ifndef PRIu32
+-#define PRIu32 "u"
+-#endif
+-
+-#ifndef PRIx32
+-#define PRIx32 "x"
+-#endif
+-
+-#ifndef PRIo32
+-#define PRIo32 "o"
+-#endif
+-
+ typedef uintmax_t timestamp_t;
+ #define PRItime PRIuMAX
+ #define parse_timestamp strtoumax

#7 lore raw

Re: [PATCH 1/1] git-compat-util.h: drop the `PRIuMAX` definition

Quoted 10 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
Quoted 2 lines

Can we likewise ditch the fallback definition for SCNuMAX? And PRIu32, etc? It seems likely any platform would either have all of them or none.

I guess that's also a C99-ism that we can use?

Thanks, both.

Here is what I have locally for now.

Yes, that looks good to me (and yes, those other format macros are in C99, too).

-Peff