Re: [PATCH] use appropriate typedefs
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:37
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] use appropriate typedefs · David Rientjes <rientjes@google.com>
- 2016-06-15 · Re: [PATCH] use appropriate typedefs · Johannes Schindelin <hidden>
- 2016-06-15 · Re: [PATCH] use appropriate typedefs · David Rientjes <rientjes@google.com>
David Rientjes [off-list ref] writes:
On Tue, 15 Aug 2006, David Rientjes wrote: Please replace the original patch with the following.
quoted hunk ↗ jump to hunk
diff --git a/builtin-apply.c b/builtin-apply.c index 9cf477c..56c5394 100644 --- a/builtin-apply.c +++ b/builtin-apply.c@@ -2097,7 +2097,7 @@ static void create_one_file(char *path, } if (errno == EEXIST) { - unsigned int nr = getpid(); + pid_t nr = getpid();
(earlier)
quoted
Since mkpath() is vararg, doesn't this make it necessary to cast its parameter several lines down?No, it is not necessary in the sense that any of these changes in this patch are necessary. But since getpid() returns pid_t, every assignment should be cast as such. pid_t can be typed as a short.
If pid_t is a short then wouldn't it be promoted to "unsigned
int" just fine, except for the failure case (but this is
getpid() we are dealing with here)? More problematic is the
case where pid_t is wider than unsigned int, in which case we
can end up truncating the return value from getpid().
But for this particular case, I do not think it matters; the
code uses getpid() to seed the loop to obtain an unused
"~number" suffix; it could have used random(3) or time(2)
instead. As long as:
newpath = mkpath("%s~%u", path, nr);
does a reasonable thing, we are Ok.
I think, however, if you change the type of nr to pid_t, you
would need to cast it like this, because mkpath is defined to be
"char *mkpath(const char *, ...)":
newpath = mkpath("%s~%u", path, (unsigned int) nr);
Or use whatever matching integral type and format letter pairs;
we seem to like "%lu" format and "(unsigned long)" in other
parts of our code.
quoted hunk ↗ jump to hunk
diff --git a/fetch-clone.c b/fetch-clone.c index 5e84c46..c5cf477 100644 --- a/fetch-clone.c +++ b/fetch-clone.c@@ -44,9 +44,8 @@ static int finish_pack(const char *pack_ for (;;) { int status, code; - int retval = waitpid(pid, &status, 0); - if (retval < 0) { + if (waitpid(pid, &status, 0) < 0) { if (errno == EINTR) continue; error("waitpid failed (%s)", strerror(errno));
Makes sense -- if pid_t is wider than int we would be in trouble.
quoted hunk ↗ jump to hunk
diff --git a/merge-index.c b/merge-index.c
Same.
quoted hunk ↗ jump to hunk
diff --git a/run-command.c b/run-command.c index ca67ee9..3bacc1b 100644 --- a/run-command.c +++ b/run-command.c@@ -25,15 +25,15 @@ int run_command_v_opt(int argc, const ch } for (;;) { int status, code; - int retval = waitpid(pid, &status, 0); + pid_t waiting = waitpid(pid, &status, 0); - if (retval < 0) { + if (waiting < 0) { if (errno == EINTR) continue;
Same.
- error("waitpid failed (%s)", strerror(retval));
+ error("waitpid failed (%s)", strerror(waiting));Shouldn't this be "strerror(errno)"? The original gets it wrong already.
quoted hunk ↗ jump to hunk
diff --git a/unpack-trees.c b/unpack-trees.c index a20639b..e496d8c 100644 --- a/unpack-trees.c +++ b/unpack-trees.c@@ -278,7 +278,7 @@ static void unlink_entry(char *name) } } -static volatile int progress_update = 0; +static volatile sig_atomic_t progress_update = 0; static void progress_interval(int signum) {
This matches the other one in builtin-pack-objects.c and makes sense. Thanks.