{"thread":{"id":"46423","subject":"[PATCH] run_processes_parallel: change confusing task_cb convention","startedAt":"2017-07-19T14:56:32Z","lastAt":"2017-07-19T17:16:53Z","messageCount":2,"participants":["Johannes Schindelin","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324766","messageId":"7151686aba3c5254bfff35bfd7cdd1d48992e99a.1500476164.git.johannes.schindelin@gmx.de","threadId":"46423","inReplyTo":null,"subject":"[PATCH] run_processes_parallel: change confusing task_cb convention","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-07-19T14:56:19Z","receivedAt":"2017-07-19T14:56:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"By declaring the task_cb parameter of type `void **`, the signature of\nthe get_next_task method suggests that the \"task-specific cookie\" can be\ndefined in that method, and the signatures of the start_failure and of\nthe task_finished methods declare that parameter of type `void *`,\nsuggesting that those methods are mere users of said cookie.\n\nThat convention makes a total lot of sense, because the tasks are pretty\nmuch dead when one of the latter two methods is called: there would be\nlittle use to reset that cookie at that point because nobody would be\nable to see the change afterwards.\n\nHowever, this is not what the code actually does. For all three methods,\nit passes the *address* of pp->children[i].data.\n\nAs reasoned above, this behavior makes no sense. So let's change the\nimplementation to adhere to the convention suggested by the signatures.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/submodule--helper.c | 2 +-\n run-command.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6abdad3294c..3a3c9ca72b6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -930,7 +930,7 @@ static int update_clone_task_finished(int result,\n \tconst struct cache_entry *ce;\n \tstruct submodule_update_clone *suc = suc_cb;\n \n-\tint *idxP = *(int**)idx_task_cb;\n+\tint *idxP = idx_task_cb;\n \tint idx = *idxP;\n \tfree(idxP);\n \ndiff --git a/run-command.c b/run-command.c\nindex 9e36151bf97..b5e6eb37c0e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1533,7 +1533,7 @@ static int pp_start_one(struct parallel_processes *pp)\n \tif (start_command(&pp->children[i].process)) {\n \t\tcode = pp->start_failure(&pp->children[i].err,\n \t\t\t\t\t pp->data,\n-\t\t\t\t\t &pp->children[i].data);\n+\t\t\t\t\t pp->children[i].data);\n \t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n \t\tstrbuf_reset(&pp->children[i].err);\n \t\tif (code)\n@@ -1601,7 +1601,7 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \n \t\tcode = pp->task_finished(code,\n \t\t\t\t\t &pp->children[i].err, pp->data,\n-\t\t\t\t\t &pp->children[i].data);\n+\t\t\t\t\t pp->children[i].data);\n \n \t\tif (code)\n \t\t\tresult = code;\n\nbase-commit: cac25fc330fc26050dcbc92c4bfff169a4848e93\n-- \n2.13.3.windows.1.13.gaf0c2223da0\n\nPublished-As: https://github.com/dscho/git/releases/tag/run-process-parallel-v1\nFetch-It-Via: git fetch https://github.com/dscho/git run-process-parallel-v1\n"},{"id":"324770","messageId":"CAGZ79kb9au53Rp5COvbbvoFToD_g4OAHQAqCFFUYyHi-EA+iAQ@mail.gmail.com","threadId":"46423","inReplyTo":"7151686aba3c5254bfff35bfd7cdd1d48992e99a.1500476164.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] run_processes_parallel: change confusing task_cb convention","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-19T17:16:47Z","receivedAt":"2017-07-19T17:16:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 19, 2017 at 7:56 AM, Johannes Schindelin\n wrote:\n> By declaring the task_cb parameter of type `void **`, the signature of\n> the get_next_task method suggests that the \"task-specific cookie\" can be\n> defined in that method, and the signatures of the start_failure and of\n> the task_finished methods declare that parameter of type `void *`,\n> suggesting that those methods are mere users of said cookie.\n>\n> That convention makes a total lot of sense, because the tasks are pretty\n> much dead when one of the latter two methods is called: there would be\n> little use to reset that cookie at that point because nobody would be\n> able to see the change afterwards.\n>\n> However, this is not what the code actually does. For all three methods,\n> it passes the *address* of pp->children[i].data.\n>\n> As reasoned above, this behavior makes no sense. So let's change the\n> implementation to adhere to the convention suggested by the signatures.\n>\n> Signed-off-by: Johannes Schindelin \n\nMy bad. Thanks for spotting and fixing!\nReviewed-by: Stefan Beller \n"}]}