Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

harry-hov
Copy link

@harry-hov harry-hov commented Nov 23, 2019

Git's code base already seems to be using PRIdMAX without any such fallback
definition for quite a while (7545941 (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 #399

@harry-hov
Copy link
Author

Hi @dscho,
I'm not sure about changes. Would like to have your review
(Solves #399)

@dscho
Copy link
Member

dscho commented Nov 23, 2019

Looks good, but I would like to see the commit message improved:

  • the oneline should start with an "area"
  • it should be in the "imperative" form, not in the past tense
  • it should mention Get rid of the PRIuMAX guard in git-compat-util.h #399 directly (to auto-close it)
  • at least to me, the text does not flow easily, and maybe that could be enhanced at the same time?

I.e.

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

Git's code base already seems to be using `PRIdMAX` without any such
fallback definition for quite a while (7545941 (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 gitgitgadget#399

Signed-off-by: Hariom Verma <[email protected]>
@harry-hov harry-hov changed the title Removed PRIuMAX guard from git-compat-util.h git-compat-util.h: drop the PRIuMAX definition Nov 24, 2019
@harry-hov
Copy link
Author

Thanks @dscho
I'll try to come up with better commit message next time

@harry-hov
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 24, 2019

Submitted as [email protected]

@@ -320,10 +320,6 @@ char *gitdirname(char *);
#define PATH_MAX 4096
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Sun, Nov 24, 2019 at 01:09:23PM +0000, Hariom Verma via GitGitGadget wrote:

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

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 607dca7534..ba710cfa6c 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -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...

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Carlo Arenas wrote (reply to this):

On Sun, Nov 24, 2019 at 9:11 AM Jeff King <[email protected]> wrote:
>
> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Carlo Arenas wrote (reply to this):

On Sun, Nov 24, 2019 at 9:40 AM Carlo Arenas <[email protected]> wrote:
>
> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <[email protected]> writes:

> On Sun, Nov 24, 2019 at 01:09:23PM +0000, Hariom Verma via GitGitGadget wrote:
>
>> 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.

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

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

> This part of the patch looks obviously correct. :) But...
>
>>  #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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes:

>> 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.
     
         Signed-off-by: Hariom Verma <[email protected]>
    +    Helped-by: Jeff King <[email protected]>
    +    [jc: tweaked both message and code, taking what peff wrote]
         Signed-off-by: Junio C Hamano <[email protected]>
     
      ## 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Nov 25, 2019 at 11:45:29AM +0900, Junio C Hamano wrote:

> Junio C Hamano <[email protected]> writes:
> 
> >> 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

@dscho
Copy link
Member

dscho commented Nov 24, 2019

I'll try to come up with better commit message next time

Umm. I will try to remember that you flat out ignore my suggestions and /submit unchanged anyway, and save myself a lot of time.

@dscho
Copy link
Member

dscho commented Nov 24, 2019

I will try to remember that you flat out ignore my suggestions and /submit unchanged anyway

Oh, but you didn't, even if your answer sounded as if you had. I would have expected a reply more like "Thank you, I used that commit message with minor edits" than "I'll do better next time."...

@harry-hov harry-hov closed this Dec 9, 2019
@harry-hov harry-hov deleted the priumax branch December 9, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants