| Message ID | patch-v2-01.24-fcdf4a2e2d9-20220719T204458Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule--helper: fix memory leaks | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Use the less verbose { 0 }-initialization syntax rather than memset() > in builtin/submodule--helper.c, this doesn't make a difference in > terms of behavior, but as we're about to modify adjacent code makes > this more consistent, and lets us avoid worrying about when the > memset() happens v.s. a "goto cleanup". > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/submodule--helper.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) These four hunks form two pairs of two hunks, one declaring (and in the new code, initializing) a variable, the other clearing (and in the new code, there is no need to) the variable. > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index fac52ade5e1..73717be957c 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1744,7 +1744,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > { > int dissociate = 0, quiet = 0, progress = 0, require_init = 0; > struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT; > - struct list_objects_filter_options filter_options; > + struct list_objects_filter_options filter_options = { 0 }; > > struct option module_clone_options[] = { > OPT_STRING(0, "prefix", &clone_data.prefix, > @@ -1786,7 +1786,6 @@ static int module_clone(int argc, const char **argv, const char *prefix) > NULL > }; > > - memset(&filter_options, 0, sizeof(filter_options)); > argc = parse_options(argc, argv, prefix, module_clone_options, > git_submodule_helper_usage, 0); > @@ -2563,7 +2562,7 @@ static int module_update(int argc, const char **argv, const char *prefix) > { > struct pathspec pathspec; > struct update_data opt = UPDATE_DATA_INIT; > - struct list_objects_filter_options filter_options; > + struct list_objects_filter_options filter_options = { 0 }; > int ret; > > struct option module_update_options[] = { > @@ -2623,7 +2622,6 @@ static int module_update(int argc, const char **argv, const char *prefix) > update_clone_config_from_gitmodules(&opt.max_jobs); > git_config(git_update_clone_config, &opt.max_jobs); > > - memset(&filter_options, 0, sizeof(filter_options)); > argc = parse_options(argc, argv, prefix, module_update_options, > git_submodule_helper_usage, 0); The fact that this patch is harder to read without wider context tells us that these two changes are good idea. Absolutely nothing happens in the first function between the declaration of the variable and memset() but it is hard to see in a patch because they are so far apart. In the second function, some things happen but they are not using the variable before they get initialized, but again, it is hard to see in a patch without wider context. The post-image of this patch allows readers easily to see that nobody uses these variables without initialization, which is a great improvement. Thanks, will queue.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index fac52ade5e1..73717be957c 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1744,7 +1744,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) { int dissociate = 0, quiet = 0, progress = 0, require_init = 0; struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT; - struct list_objects_filter_options filter_options; + struct list_objects_filter_options filter_options = { 0 }; struct option module_clone_options[] = { OPT_STRING(0, "prefix", &clone_data.prefix, @@ -1786,7 +1786,6 @@ static int module_clone(int argc, const char **argv, const char *prefix) NULL }; - memset(&filter_options, 0, sizeof(filter_options)); argc = parse_options(argc, argv, prefix, module_clone_options, git_submodule_helper_usage, 0); @@ -2563,7 +2562,7 @@ static int module_update(int argc, const char **argv, const char *prefix) { struct pathspec pathspec; struct update_data opt = UPDATE_DATA_INIT; - struct list_objects_filter_options filter_options; + struct list_objects_filter_options filter_options = { 0 }; int ret; struct option module_update_options[] = { @@ -2623,7 +2622,6 @@ static int module_update(int argc, const char **argv, const char *prefix) update_clone_config_from_gitmodules(&opt.max_jobs); git_config(git_update_clone_config, &opt.max_jobs); - memset(&filter_options, 0, sizeof(filter_options)); argc = parse_options(argc, argv, prefix, module_update_options, git_submodule_helper_usage, 0);
Use the less verbose { 0 }-initialization syntax rather than memset() in builtin/submodule--helper.c, this doesn't make a difference in terms of behavior, but as we're about to modify adjacent code makes this more consistent, and lets us avoid worrying about when the memset() happens v.s. a "goto cleanup". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)