| Message ID | patch-1.1-0aa4523ab6e-20210909T130849Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-tool run-command: fix confusing init pattern | expand |
Hi Ævar, the commit title is misleading: it suggests that there is a bug that needs to be fixed. The idea of the patch, however, is to avoid redundant code, and if described that way, the patch is a lot better for it. On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > In be5d88e1128 (test-tool run-command: learn to run (parts of) the > testsuite, 2019-10-04) an init pattern was added that would use > TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd > then set the "dup" on the two string lists. Our setting of "next" to > "-1" thus did nothing, we'd reset it to "0" before using it. > > Let's just use the init macro for the STRING_LIST members, we can then > remove the already redundant memset(). > > Note that while we compile this code, there's no in-tree user for the > "testsuite" target being modified here anymore, see the discussion at > and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1]. > > 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/ > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > This patch is the immediate reason for why I submitted > https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/, > since Johannes would prefer to keep it let's fix this init pattern. The diff does too many things, some of which are your purely personal preferences and do not actually need to be changed. This is a much more to-the-point diff: -- snip -- diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 7ae03dc7123..14c57365e76 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -61,7 +61,7 @@ struct testsuite { int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; }; #define TESTSUITE_INIT \ - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 } + { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 } static int next_test(struct child_process *cp, struct strbuf *err, void *cb, void **task_cb) @@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv) OPT_END() }; - memset(&suite, 0, sizeof(suite)); - suite.tests.strdup_strings = suite.failed.strdup_strings = 1; - argc = parse_options(argc, argv, NULL, options, testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION); -- snap -- I would strongly suggest to use this diff instead. Ciao, Johannes > > t/helper/test-run-command.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c > index 7ae03dc7123..8e42516bdc1 100644 > --- a/t/helper/test-run-command.c > +++ b/t/helper/test-run-command.c > @@ -56,12 +56,15 @@ static int task_finished(int result, > } > > struct testsuite { > - struct string_list tests, failed; > + struct string_list tests; > + struct string_list failed; > int next; > int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; > }; > -#define TESTSUITE_INIT \ > - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 } > +#define TESTSUITE_INIT { \ > + .tests = STRING_LIST_INIT_DUP, \ > + .failed = STRING_LIST_INIT_DUP, \ > +} > > static int next_test(struct child_process *cp, struct strbuf *err, void *cb, > void **task_cb) > @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv) > OPT_END() > }; > > - memset(&suite, 0, sizeof(suite)); > - suite.tests.strdup_strings = suite.failed.strdup_strings = 1; > - > argc = parse_options(argc, argv, NULL, options, > testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION); > > -- > 2.33.0.867.g88ec4638586 > >
On Fri, Sep 10 2021, Johannes Schindelin wrote: > Hi Ævar, > > the commit title is misleading: it suggests that there is a bug that needs > to be fixed. I picked "confusing" because this doesn't impact the end-state of the program, it's just confusing to do: x = -1 Followed by: x = 0 Which the reader might wonder about, only to find that the initial assignment wasn't needed or used for anything. > The idea of the patch, however, is to avoid redundant code, and if > described that way, the patch is a lot better for it. > > On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > >> In be5d88e1128 (test-tool run-command: learn to run (parts of) the >> testsuite, 2019-10-04) an init pattern was added that would use >> TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd >> then set the "dup" on the two string lists. Our setting of "next" to >> "-1" thus did nothing, we'd reset it to "0" before using it. >> >> Let's just use the init macro for the STRING_LIST members, we can then >> remove the already redundant memset(). >> >> Note that while we compile this code, there's no in-tree user for the >> "testsuite" target being modified here anymore, see the discussion at >> and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1]. >> >> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/ >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> >> This patch is the immediate reason for why I submitted >> https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/, >> since Johannes would prefer to keep it let's fix this init pattern. > > The diff does too many things, some of which are your purely personal > preferences and do not actually need to be changed. This is a much more > to-the-point diff: We've been slowly converting everything to designated initializers. It seems to make sense to just do that if the line is being touched anyway. For instance my d385784f89b (fsck.h: use designed initializers for FSCK_OPTIONS_{DEFAULT,STRICT}, 2021-03-28) was part of a series whose initial version just changed one field at a time as you're doing below, but during early review I was asked just to use designated initializers already. But yes, it is strictly unrelated. It's a judgement call when to do cleanups on lines you touch while you're at it, v.s. turning one patch into a potential series of really tiny changes. Then there's the change of "struct foo x, y" to "struct foo x;\nstruct foo y;\n" above. I changed that because that tends to be the usual style, it also preempts feedback / reviewers having to look at the code out-of-bounds to see what the struct looks like in the default diff -U<n>. So, in advance addressing another type of question, or needing to describe the context, as you brought up in https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091222260.59@tvgsbejvaqbjf.bet/ :) > -- snip -- > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c > index 7ae03dc7123..14c57365e76 100644 > --- a/t/helper/test-run-command.c > +++ b/t/helper/test-run-command.c > @@ -61,7 +61,7 @@ struct testsuite { > int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; > }; > #define TESTSUITE_INIT \ > - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 } > + { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 } > > static int next_test(struct child_process *cp, struct strbuf *err, void *cb, > void **task_cb) > @@ -142,9 +142,6 @@ static int testsuite(int argc, const char **argv) > OPT_END() > }; > > - memset(&suite, 0, sizeof(suite)); > - suite.tests.strdup_strings = suite.failed.strdup_strings = 1; > - > argc = parse_options(argc, argv, NULL, options, > testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION); > -- snap -- > > I would strongly suggest to use this diff instead. > > Ciao, > Johannes > > >> >> t/helper/test-run-command.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c >> index 7ae03dc7123..8e42516bdc1 100644 >> --- a/t/helper/test-run-command.c >> +++ b/t/helper/test-run-command.c >> @@ -56,12 +56,15 @@ static int task_finished(int result, >> } >> >> struct testsuite { >> - struct string_list tests, failed; >> + struct string_list tests; >> + struct string_list failed; >> int next; >> int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; >> }; >> -#define TESTSUITE_INIT \ >> - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 } >> +#define TESTSUITE_INIT { \ >> + .tests = STRING_LIST_INIT_DUP, \ >> + .failed = STRING_LIST_INIT_DUP, \ >> +} >> >> static int next_test(struct child_process *cp, struct strbuf *err, void *cb, >> void **task_cb) >> @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv) >> OPT_END() >> }; >> >> - memset(&suite, 0, sizeof(suite)); >> - suite.tests.strdup_strings = suite.failed.strdup_strings = 1; >> - >> argc = parse_options(argc, argv, NULL, options, >> testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION); >> >> -- >> 2.33.0.867.g88ec4638586 >> >>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> The diff does too many things, some of which are your purely personal >> preferences and do not actually need to be changed. This is a much more >> to-the-point diff: > > We've been slowly converting everything to designated initializers. It > seems to make sense to just do that if the line is being touched anyway. Perhaps a preliminary clean-up patch is called for in such a case? I do not think anybody can immediately see what the difference between the old -1 and the new 0 in TESTSUITE_INIT macro means in Dscho's alternative, but if we had a preliminary clean-up whose sole change is to use designated initializers, the real "to-the-point" step would become much easier to see which member that used to be initialized to -1 is now getting zero-initialized. And yes, changing the initializer style *and* the values the members are initialized to in a same patch is much worse than sticking to the style of the unreadable original. It buries the real change in the noise. >> #define TESTSUITE_INIT \ >> - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 } >> + { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 7ae03dc7123..8e42516bdc1 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -56,12 +56,15 @@ static int task_finished(int result, } struct testsuite { - struct string_list tests, failed; + struct string_list tests; + struct string_list failed; int next; int quiet, immediate, verbose, verbose_log, trace, write_junit_xml; }; -#define TESTSUITE_INIT \ - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 } +#define TESTSUITE_INIT { \ + .tests = STRING_LIST_INIT_DUP, \ + .failed = STRING_LIST_INIT_DUP, \ +} static int next_test(struct child_process *cp, struct strbuf *err, void *cb, void **task_cb) @@ -142,9 +145,6 @@ static int testsuite(int argc, const char **argv) OPT_END() }; - memset(&suite, 0, sizeof(suite)); - suite.tests.strdup_strings = suite.failed.strdup_strings = 1; - argc = parse_options(argc, argv, NULL, options, testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
In be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04) an init pattern was added that would use TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd then set the "dup" on the two string lists. Our setting of "next" to "-1" thus did nothing, we'd reset it to "0" before using it. Let's just use the init macro for the STRING_LIST members, we can then remove the already redundant memset(). Note that while we compile this code, there's no in-tree user for the "testsuite" target being modified here anymore, see the discussion at and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1]. 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- This patch is the immediate reason for why I submitted https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/, since Johannes would prefer to keep it let's fix this init pattern. t/helper/test-run-command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)