David Soria ParraThis small patch series fixes some compile warnings on OpenSolaris/Solaris. As pid_t is a long on Solaris, I changed the output of fprintf&co to longs and cast pids to long, so that we are safe on the solaris
threads / patch / 15220
patch, 2 partsImprove portability for OpenSolaris
Subject: [PATCH 0/2] Improve portability for OpenSolaris
David Soria ParraDavid Soria Parra[PATCH 1/2] Improvate portability: Display pid_t's always as long
From: David Soria Parra <dsp@php.net>
Some systems (like e.g. OpenSolaris) define pid_t as long, therefore all our sprintf that use %i cause a compiler warning beacuse if the implicit long->int cast. So to make sure that we fit the limits we display pids as longs and cast them explicitly.
Signed-off-by: David Soria Parra <dsp@php.net> --- builtin-commit.c | 2 +- builtin-fetch-pack.c | 2 +- daemon.c | 2 +- fast-import.c | 6 +++--- receive-pack.c | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
Show changes to 5 files +7 −7
builtin-commit.c, builtin-fetch-pack.c, daemon.c, fast-import.c, receive-pack.c
diff --git a/builtin-commit.c b/builtin-commit.c index 649c8be..b4c940f 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -320,7 +320,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix) die("unable to write new_index file"); fd = hold_lock_file_for_update(&false_lock, - git_path("next-index-%d", getpid()), 1); + git_path("next-index-%ld", (long) getpid()), 1); create_base_index(); add_remove_files(&partial); diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c index 273239a..46bb3e2 100644 --- a/builtin-fetch-pack.c +++ b/builtin-fetch-pack.c @@ -540,7 +540,7 @@ static int get_pack(int xd[2], char **pack_lockfile) *av++ = "--fix-thin"; if (args.lock_pack || unpack_limit) { int s = sprintf(keep_arg, - "--keep=fetch-pack %d on ", getpid()); + "--keep=fetch-pack %ld on ", (long) getpid()); if (gethostname(keep_arg + s, sizeof(keep_arg) - s)) strcpy(keep_arg + s, "localhost"); *av++ = keep_arg; diff --git a/daemon.c b/daemon.c index 23278e2..1e5cb58 100644 --- a/daemon.c +++ b/daemon.c @@ -923,7 +923,7 @@ static void store_pid(const char *path) FILE *f = fopen(path, "w"); if (!f) die("cannot open pid file %s: %s", path, strerror(errno)); - if (fprintf(f, "%d\n", getpid()) < 0 || fclose(f) != 0) + if (fprintf(f, "%ld\n", (long) getpid()) < 0 || fclose(f) != 0) die("failed to write pid file %s: %s", path, strerror(errno)); } diff --git a/fast-import.c b/fast-import.c index 7089e6f..e04ed94 100644 --- a/fast-import.c +++ b/fast-import.c @@ -376,7 +376,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *); static void write_crash_report(const char *err) { - char *loc = git_path("fast_import_crash_%d", getpid()); + char *loc = git_path("fast_import_crash_%ld", (long) getpid()); FILE *rpt = fopen(loc, "w"); struct branch *b; unsigned long lu; @@ -390,8 +390,8 @@ static void write_crash_report(const char *err) fprintf(stderr, "fast-import: dumping crash report to %s\n", loc); fprintf(rpt, "fast-import crash report:\n"); - fprintf(rpt, " fast-import process: %d\n", getpid()); - fprintf(rpt, " parent process : %d\n", getppid()); + fprintf(rpt, " fast-import process: %ld\n", (long) getpid()); + fprintf(rpt, " parent process : %ld\n", (long) getppid()); fprintf(rpt, " at %s\n", show_date(time(NULL), 0, DATE_LOCAL)); fputc('\n', rpt); diff --git a/receive-pack.c b/receive-pack.c index d44c19e..0a82f73 100644 --- a/receive-pack.c +++ b/receive-pack.c @@ -407,7 +407,7 @@ static const char *unpack(void) char keep_arg[256]; struct child_process ip; - s = sprintf(keep_arg, "--keep=receive-pack %i on ", getpid()); + s = sprintf(keep_arg, "--keep=receive-pack %li on ", (long) getpid()); if (gethostname(keep_arg + s, sizeof(keep_arg) - s)) strcpy(keep_arg + s, "localhost");
-- 1.6.0.174.gd789c
David Soria Parra[PATCH 2/2] Improve portability: Avoid SS constant as it is already defined
From: David Soria Parra <dsp@php.net>
Constants that have the names of CPU registers are already defined in OpenSolaris's sys/regset.h. This causes a warning as we try to (re)define SS in ctype.c. So we just use another name.
Signed-off-by: David Soria Parra <dsp@php.net> --- ctype.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to ctype.c +3 −3
diff --git a/ctype.c b/ctype.c index ee06eb7..6ed2ba2 100644 --- a/ctype.c +++ b/ctype.c @@ -5,14 +5,14 @@ */ #include "cache.h" -#define SS GIT_SPACE +#define SP GIT_SPACE #define AA GIT_ALPHA #define DD GIT_DIGIT unsigned char sane_ctype[256] = { - 0, 0, 0, 0, 0, 0, 0, 0, 0, SS, SS, 0, 0, SS, 0, 0, /* 0-15 */ + 0, 0, 0, 0, 0, 0, 0, 0, 0, SP, SP, 0, 0, SP, 0, 0, /* 0-15 */ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 16-15 */ - SS, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 32-15 */ + SP, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 32-15 */ DD, DD, DD, DD, DD, DD, DD, DD, DD, DD, 0, 0, 0, 0, 0, 0, /* 48-15 */ 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, /* 64-15 */ AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, 0, 0, 0, 0, 0, /* 80-15 */
-- 1.6.0.174.gd789c
Junio C HamanoRe: [PATCH 2/2] Improve portability: Avoid SS constant as it is already defined
David Soria Parra <sn_@gmx.net> writes:
Show 5 quoted lines
> From: David Soria Parra <dsp@php.net> > > Constants that have the names of CPU registers are already defined > in OpenSolaris's sys/regset.h. This causes a warning as we try to > (re)define SS in ctype.c. So we just use another name.
I do not mind this _particular_ workaround per-se, but I have to wonder what happens the next time some random other platform has "SP" defined in a random header file.
First of all, why are you including <sys/regset.h>? We certainly don't include from any of our header or source files. And second of all, why is the indirect inclusion of that header file by some standard header file we do include cause the namespace to get poluted with "SS" symbol?
Junio C HamanoRe: [PATCH 2/2] Improve portability: Avoid SS constant as it is already defined
Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> David Soria Parra <sn_@gmx.net> writes: > >> From: David Soria Parra <dsp@php.net> >> >> Constants that have the names of CPU registers are already defined >> in OpenSolaris's sys/regset.h. This causes a warning as we try to >> (re)define SS in ctype.c. So we just use another name. > > I do not mind this _particular_ workaround per-se, but I have to wonder > what happens the next time some random other platform has "SP" defined in > a random header file.
If we are doing an workaround, how about doing it this way instead?
ctype.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
Show changes to diff +5 −0
diff --git i/ctype.c w/ctype.c index ee06eb7..d2bd38e 100644 --- i/ctype.c +++ w/ctype.c @@ -5,6 +5,11 @@ */ #include "cache.h" +/* Just so that no insane platform contaminates the namespace with these symbols */ +#undef SS +#undef AA +#undef DD + #define SS GIT_SPACE #define AA GIT_ALPHA #define DD GIT_DIGIT
Re: [PATCH 2/2] Improve portability: Avoid SS constant as it is already defined
Should be fine as well.
Junio C Hamano wrote:
Show 34 quoted lines
> Junio C Hamano <gitster@pobox.com> writes: > >> David Soria Parra <sn_@gmx.net> writes: >> >>> From: David Soria Parra <dsp@php.net> >>> >>> Constants that have the names of CPU registers are already defined >>> in OpenSolaris's sys/regset.h. This causes a warning as we try to >>> (re)define SS in ctype.c. So we just use another name. >> I do not mind this _particular_ workaround per-se, but I have to wonder >> what happens the next time some random other platform has "SP" defined in >> a random header file. > > If we are doing an workaround, how about doing it this way instead? > > ctype.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git i/ctype.c w/ctype.c > index ee06eb7..d2bd38e 100644 > --- i/ctype.c > +++ w/ctype.c > @@ -5,6 +5,11 @@ > */ > #include "cache.h" > > +/* Just so that no insane platform contaminates the namespace with these symbols */ > +#undef SS > +#undef AA > +#undef DD > + > #define SS GIT_SPACE > #define AA GIT_ALPHA > #define DD GIT_DIGIT
Junio C HamanoRe: [PATCH 1/2] Improvate portability: Display pid_t's always as long
David Soria Parra <sn_@gmx.net> writes:
> Some systems (like e.g. OpenSolaris) define pid_t as long, > therefore all our sprintf that use %i cause a compiler warning > beacuse if the implicit long->int cast. So to make sure that > we fit the limits we display pids as longs and cast them explicitly.
This patch just makes one wonder what needs to happen when the next random platform has pid_t as long long or int32_t or whatever signed integral type that was picked arbitrarily by the platform.
I think these *printf()s are mostly for informational purposes and if you favor minimum change, you might be better off casting it to "int" without changing the format specifiers.
On the other hand, if you are shooting for maximum compatibility perhaps you may want to cast it to "intmax_t" and format as such.
Casting to "long" does not make much sense from either perspective, does it?
Re: [PATCH 1/2] Improvate portability: Display pid_t's always as long
> On the other hand, if you are shooting for maximum compatibility perhaps > you may want to cast it to "intmax_t" and format as such. Yes, good point, casting to long isn't enough. I think it's a good approach to cast the pids to intmax_t as pids are also used in git_path() and therefore might result in equal paths for separate processes if the pid is higher than int.
so here is an updated patch:
From da5519b3ae5ce84c703aeaab2bc4ea363897c334 Mon Sep 17 00:00:00 2001 From: David Soria Parra <dsp at> Date: Fri, 29 Aug 2008 01:19:43 +0200 Subject: [PATCH] Improvate portability: Cast pid_t's to intmax_t
Some systems (like e.g. OpenSolaris) define pid_t as long, therefore all our sprintf that use %i cause a compiler warning beacuse if the implicit long->int cast. So to make sure that we fit the limits we display pids as intmax_t and cast them explicitly.
Signed-off-by: David Soria Parra <dsp@php.net> --- builtin-commit.c | 2 +- builtin-fetch-pack.c | 2 +- daemon.c | 6 +++--- fast-import.c | 6 +++--- receive-pack.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-)
Show changes to 5 files +9 −9
builtin-commit.c, builtin-fetch-pack.c, daemon.c, fast-import.c, receive-pack.c
diff --git a/builtin-commit.c b/builtin-commit.c index c870037..90ef3d5 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -320,7 +320,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix) die("unable to write new_index file"); fd = hold_lock_file_for_update(&false_lock, - git_path("next-index-%d", getpid()), 1); + git_path("next-index-%jd", (intmax_t) getpid()), 1); create_base_index(); add_remove_files(&partial); diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c index 273239a..91616e7 100644 --- a/builtin-fetch-pack.c +++ b/builtin-fetch-pack.c @@ -540,7 +540,7 @@ static int get_pack(int xd[2], char **pack_lockfile) *av++ = "--fix-thin"; if (args.lock_pack || unpack_limit) { int s = sprintf(keep_arg, - "--keep=fetch-pack %d on ", getpid()); + "--keep=fetch-pack %jd on ", (intmax_t) getpid()); if (gethostname(keep_arg + s, sizeof(keep_arg) - s)) strcpy(keep_arg + s, "localhost"); *av++ = keep_arg; diff --git a/daemon.c b/daemon.c index 23278e2..6081986 100644 --- a/daemon.c +++ b/daemon.c @@ -86,7 +86,7 @@ static void logreport(int priority, const char *err, va_list params) * Since stderr is set to linebuffered mode, the * logging of different processes will not overlap */ - fprintf(stderr, "[%d] ", (int)getpid()); + fprintf(stderr, "[%jd] ", (intmax_t)getpid()); vfprintf(stderr, err, params); fputc('\n', stderr); } @@ -658,7 +658,7 @@ static void check_dead_children(void) remove_child(pid); if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0)) dead = " (with error)"; - loginfo("[%d] Disconnected%s", (int)pid, dead); + loginfo("[%jd] Disconnected%s", (intmax_t)pid, dead); } } @@ -923,7 +923,7 @@ static void store_pid(const char *path) FILE *f = fopen(path, "w"); if (!f) die("cannot open pid file %s: %s", path, strerror(errno)); - if (fprintf(f, "%d\n", getpid()) < 0 || fclose(f) != 0) + if (fprintf(f, "%jd\n", (intmax_t) getpid()) < 0 || fclose(f) != 0) die("failed to write pid file %s: %s", path, strerror(errno)); } diff --git a/fast-import.c b/fast-import.c index 7089e6f..e3a6510 100644 --- a/fast-import.c +++ b/fast-import.c @@ -376,7 +376,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *); static void write_crash_report(const char *err) { - char *loc = git_path("fast_import_crash_%d", getpid()); + char *loc = git_path("fast_import_crash_%jd", (intmax_t) getpid()); FILE *rpt = fopen(loc, "w"); struct branch *b; unsigned long lu; @@ -390,8 +390,8 @@ static void write_crash_report(const char *err) fprintf(stderr, "fast-import: dumping crash report to %s\n", loc); fprintf(rpt, "fast-import crash report:\n"); - fprintf(rpt, " fast-import process: %d\n", getpid()); - fprintf(rpt, " parent process : %d\n", getppid()); + fprintf(rpt, " fast-import process: %jd\n", (intmax_t) getpid()); + fprintf(rpt, " parent process : %jd\n", (intmax_t) getppid()); fprintf(rpt, " at %s\n", show_date(time(NULL), 0, DATE_LOCAL)); fputc('\n', rpt); diff --git a/receive-pack.c b/receive-pack.c index d44c19e..ec770d0 100644 --- a/receive-pack.c +++ b/receive-pack.c @@ -407,7 +407,7 @@ static const char *unpack(void) char keep_arg[256]; struct child_process ip; - s = sprintf(keep_arg, "--keep=receive-pack %i on ", getpid()); + s = sprintf(keep_arg, "--keep=receive-pack %ji on ", (intmax_t) getpid()); if (gethostname(keep_arg + s, sizeof(keep_arg) - s)) strcpy(keep_arg + s, "localhost");
-- 1.6.0.174.gd789c
Junio C HamanoRe: [PATCH 1/2] Improvate portability: Display pid_t's always as long
David Soria Parra <dsoria@gmx.net> writes:
Show 7 quoted lines
>> On the other hand, if you are shooting for maximum compatibility perhaps >> you may want to cast it to "intmax_t" and format as such. > Yes, good point, casting to long isn't enough. I think it's a good approach to cast the pids to intmax_t as pids > are also used in git_path() and therefore might result in equal paths for separate processes if > the pid is higher than int. > > so here is an updated patch:
Please wrap lines to reasonable length (e.g. 70-76 cols).
Please move commentary like this that clarifies context of the patch submission to after three-dashes (emulate patches from people with good manners).
> From da5519b3ae5ce84c703aeaab2bc4ea363897c334 Mon Sep 17 00:00:00 2001
Especially, don't paste this line.
> From: David Soria Parra <dsp at> > Date: Fri, 29 Aug 2008 01:19:43 +0200 > Subject: [PATCH] Improvate portability: Cast pid_t's to intmax_t
"Improvate"?
Including these in your message is not very useful. These in-body headers are used to override what can be read from the real headers of the e-mail message, but you do not have a valid e-mail address here!
Show 12 quoted lines
> Some systems (like e.g. OpenSolaris) define pid_t as long,
> ...
> diff --git a/builtin-commit.c b/builtin-commit.c
> index c870037..90ef3d5 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -320,7 +320,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
> die("unable to write new_index file");
>
> fd = hold_lock_file_for_update(&false_lock,
> - git_path("next-index-%d", getpid()), 1);
> + git_path("next-index-%jd", (intmax_t) getpid()), 1);Some systems we support do not have %j width specifier. I'd suggest casting up to uintmax_t and format with PRIuMAX, which we do define a substitute for portability.