From: Phillip Wood <phillip.wood123@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
Calvin Wan <calvinwan@google.com>,
Emily Shaffer <emilyshaffer@google.com>
Subject: Re: [PATCH v2 08/22] run-command.c: use C99 "for (TYPE VAR = ..." syntax where useful
Date: Wed, 12 Oct 2022 14:04:35 +0100 [thread overview]
Message-ID: <ae08523f-13ac-d9aa-b787-f136a88b5804@gmail.com> (raw)
In-Reply-To: <patch-v2-08.22-279b0430c5d-20221012T084850Z-avarab@gmail.com>
Hi Ævar
On 12/10/2022 10:01, Ævar Arnfjörð Bjarmason wrote:
> Refactor code in run-command.c where the "i" iteration variable is
> being compared to an unsigned type, or where it's being shadowed.
>
> In a subsequent commit the type of the "n" variable will be changed,
> this also helps to avoid some of the warnings we've had under
> "DEVOPTS=extra-all" since 8d133a4653a (strvec: use size_t to store nr
> and alloc, 2021-09-11).
>
> Per the thread ending at [1] we already have this C99 syntax in the
> v2.38.0 release, per 6563706568b (CodingGuidelines: give deadline for
> "for (int i = 0; ...", 2022-03-30) we should re-visit the wider use of
> this syntax for November 2022, meaning within the window of v2.39.0.
>
> As of writing it's earlier than that deadline and per [1] we want to
> "avoid open[ing] the floodgate and deliberately add more [this C99
> syntax]". But in this case the use of the syntax solves a real problem
> of conflating types, which we'd otherwise need to avoid by using
> differently named iteration variables (and the associated larger
> refactoring).
>
> 1. https://lore.kernel.org/git/xmqqy1wgwkbj.fsf@gitster.g/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> run-command.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index bd45828fe2c..7b27e4de78d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -443,7 +443,6 @@ static char **prep_childenv(const char *const *deltaenv)
> struct string_list env = STRING_LIST_INIT_DUP;
> struct strbuf key = STRBUF_INIT;
> const char *const *p;
> - int i;
>
> /* Construct a sorted string list consisting of the current environ */
> for (p = (const char *const *) environ; p && *p; p++) {
> @@ -476,7 +475,7 @@ static char **prep_childenv(const char *const *deltaenv)
>
> /* Create an array of 'char *' to be used as the childenv */
> ALLOC_ARRAY(childenv, env.nr + 1);
> - for (i = 0; i < env.nr; i++)
> + for (size_t i = 0; i < env.nr; i++)
In this case we're changing the type to avoid a signed/unsigned
comparison. We could achieve this by just changing the type of i above.
> childenv[i] = env.items[i].util;
> childenv[env.nr] = NULL;
>
> @@ -581,7 +580,6 @@ static void trace_add_env(struct strbuf *dst, const char *const *deltaenv)
> {
> struct string_list envs = STRING_LIST_INIT_DUP;
> const char *const *e;
> - int i;
> int printed_unset = 0;
>
> /* Last one wins, see run-command.c:prep_childenv() for context */
> @@ -599,7 +597,7 @@ static void trace_add_env(struct strbuf *dst, const char *const *deltaenv)
> }
>
> /* "unset X Y...;" */
> - for (i = 0; i < envs.nr; i++) {
> + for (size_t i = 0; i < envs.nr; i++) {
Ditto
> const char *var = envs.items[i].string;
> const char *val = envs.items[i].util;
>
> @@ -616,7 +614,7 @@ static void trace_add_env(struct strbuf *dst, const char *const *deltaenv)
> strbuf_addch(dst, ';');
>
> /* ... followed by "A=B C=D ..." */
> - for (i = 0; i < envs.nr; i++) {
> + for (size_t i = 0; i < envs.nr; i++) {
Ditto
> const char *var = envs.items[i].string;
> const char *val = envs.items[i].util;
> const char *oldval;
> @@ -1789,7 +1787,7 @@ void run_processes_parallel(int n,
> task_finished_fn task_finished,
> void *pp_cb)
> {
> - int i, code;
> + int code;
> int output_timeout = 100;
> int spawn_cap = 4;
> int ungroup = run_processes_parallel_ungroup;
> @@ -1801,7 +1799,7 @@ void run_processes_parallel(int n,
> pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
> ungroup);
> while (1) {
> - for (i = 0;
> + for (int i = 0;
Here we are moving the declaration to the loop.
Am I missing something, I don't understand how these changes match this
description in the commit message
> But in this case the use of the syntax solves a real problem
> of conflating types, which we'd otherwise need to avoid by using
> differently named iteration variables (and the associated larger
> refactoring).
Where are the cases where we'd need differently named variables if we
did not use this syntax. In each case we delete the old declaration.
While I can see the point in avoiding the signed/unsigned comparisons I
don't think it is strictly related to the topic of this series.
Best Wishes
Phillip
> i < spawn_cap && !pp.shutdown &&
> pp.nr_processes < pp.max_processes;
> i++) {
next prev parent reply other threads:[~2022-10-12 13:04 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-30 11:27 [PATCH 00/15] run-command API: pass functions & opts via struct Ævar Arnfjörð Bjarmason
2022-09-30 11:27 ` [PATCH 01/15] hook tests: fix redirection logic error in 96e7225b310 Ævar Arnfjörð Bjarmason
2022-09-30 11:27 ` [PATCH 02/15] submodule tests: reset "trace.out" between "grep" invocations Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 03/15] run-command tests: test stdout of run_command_parallel() Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 04/15] run-command test helper: use "else if" pattern Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 05/15] run-command tests: use "return", not "exit" Ævar Arnfjörð Bjarmason
2022-10-07 9:24 ` Phillip Wood
2022-09-30 11:28 ` [PATCH 06/15] run-command API: have "run_processes_parallel{,_tr2}()" return void Ævar Arnfjörð Bjarmason
2022-10-07 9:43 ` Phillip Wood
2022-09-30 11:28 ` [PATCH 07/15] run-command API: make "jobs" parameter an "unsigned int" Ævar Arnfjörð Bjarmason
2022-10-04 17:41 ` Calvin Wan
2022-10-07 9:53 ` Phillip Wood
2022-09-30 11:28 ` [PATCH 08/15] run-command API: don't fall back on online_cpus() Ævar Arnfjörð Bjarmason
2022-10-07 9:51 ` Phillip Wood
2022-09-30 11:28 ` [PATCH 09/15] run-command.c: add an initializer for "struct parallel_processes" Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 10/15] run-command API: add nascent "struct run_process_parallel_opts" Ævar Arnfjörð Bjarmason
2022-10-07 9:55 ` Phillip Wood
2022-09-30 11:28 ` [PATCH 11/15] run-command API: make run_process_parallel{,_tr2}() thin wrappers Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 12/15] run-command API: have run_process_parallel() take an "opts" struct Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 13/15] run-command API: move *_tr2() users to "run_processes_parallel()" Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 14/15] run-command.c: don't copy *_fn to "struct parallel_processes" Ævar Arnfjörð Bjarmason
2022-09-30 11:28 ` [PATCH 15/15] run-command.c: don't copy "ungroup" " Ævar Arnfjörð Bjarmason
2022-10-04 16:12 ` [PATCH 00/15] run-command API: pass functions & opts via struct Calvin Wan
2022-10-07 9:59 ` Phillip Wood
2022-10-07 16:46 ` Junio C Hamano
2022-10-12 9:01 ` [PATCH v2 00/22] " Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 01/22] hook tests: fix redirection logic error in 96e7225b310 Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 02/22] submodule tests: reset "trace.out" between "grep" invocations Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 03/22] run-command tests: test stdout of run_command_parallel() Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 04/22] run-command test helper: use "else if" pattern Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 05/22] run-command API: have "run_processes_parallel{,_tr2}()" return void Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 06/22] run-command tests: use "return", not "exit" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 07/22] run-command.c: remove dead assignment in while-loop Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 08/22] run-command.c: use C99 "for (TYPE VAR = ..." syntax where useful Ævar Arnfjörð Bjarmason
2022-10-12 13:04 ` Phillip Wood [this message]
2022-10-12 16:05 ` Junio C Hamano
2022-10-12 9:01 ` [PATCH v2 09/22] run-command API: make "n" parameter a "size_t" Ævar Arnfjörð Bjarmason
2022-10-12 13:09 ` Phillip Wood
2022-10-12 9:01 ` [PATCH v2 10/22] run-command API: don't fall back on online_cpus() Ævar Arnfjörð Bjarmason
2022-10-12 13:14 ` Phillip Wood
2022-10-12 9:01 ` [PATCH v2 11/22] run-command.c: use designated init for pp_init(), add "const" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 12/22] run-command API: add nascent "struct run_process_parallel_opts" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 13/22] run-command API: make run_process_parallel{,_tr2}() thin wrappers Ævar Arnfjörð Bjarmason
2022-10-12 13:23 ` Phillip Wood
2022-10-12 9:01 ` [PATCH v2 14/22] run-command API: have run_process_parallel() take an "opts" struct Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 15/22] run-command API: move *_tr2() users to "run_processes_parallel()" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 16/22] run-command.c: make "struct parallel_processes" const if possible Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 17/22] run-command.c: don't copy *_fn to "struct parallel_processes" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 18/22] run-command.c: don't copy "ungroup" " Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 19/22] run-command.c: don't copy "data" " Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 20/22] run-command.c: use "opts->processes", not "pp->max_processes" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 21/22] run-command.c: pass "opts" further down, and use "opts->processes" Ævar Arnfjörð Bjarmason
2022-10-12 9:01 ` [PATCH v2 22/22] run-command.c: remove "pp->max_processes", add "const" to signal() handler Ævar Arnfjörð Bjarmason
2022-10-12 18:58 ` Ævar Arnfjörð Bjarmason
2022-10-12 13:39 ` [PATCH v2 00/22] run-command API: pass functions & opts via struct Phillip Wood
2022-10-12 21:02 ` [PATCH v3 00/15] " Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 01/15] run-command test helper: use "else if" pattern Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 02/15] run-command API: have "run_processes_parallel{,_tr2}()" return void Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 03/15] run-command tests: use "return", not "exit" Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 04/15] run-command API: make "n" parameter a "size_t" Ævar Arnfjörð Bjarmason
2022-10-14 9:30 ` Phillip Wood
2022-10-12 21:02 ` [PATCH v3 05/15] run-command API: don't fall back on online_cpus() Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 06/15] run-command.c: use designated init for pp_init(), add "const" Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 07/15] run-command API: have run_process_parallel() take an "opts" struct Ævar Arnfjörð Bjarmason
2022-10-14 9:50 ` Phillip Wood
2022-10-12 21:02 ` [PATCH v3 08/15] run-command API: move *_tr2() users to "run_processes_parallel()" Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 09/15] run-command.c: make "struct parallel_processes" const if possible Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 10/15] run-command.c: don't copy *_fn to "struct parallel_processes" Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 11/15] run-command.c: don't copy "ungroup" " Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 12/15] run-command.c: don't copy "data" " Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 13/15] run-command.c: use "opts->processes", not "pp->max_processes" Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 14/15] run-command.c: pass "opts" further down, and use "opts->processes" Ævar Arnfjörð Bjarmason
2022-10-12 21:02 ` [PATCH v3 15/15] run-command.c: remove "max_processes", add "const" to signal() handler Ævar Arnfjörð Bjarmason
2022-10-13 22:02 ` Glen Choo
2022-10-13 19:19 ` [PATCH v3 00/15] run-command API: pass functions & opts via struct Calvin Wan
2022-10-13 20:17 ` Junio C Hamano
2022-10-14 10:00 ` Phillip Wood
2022-10-14 14:50 ` Ævar Arnfjörð Bjarmason
2022-10-14 15:53 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ae08523f-13ac-d9aa-b787-f136a88b5804@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=avarab@gmail.com \
--cc=calvinwan@google.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).