Re: [PATCH 2/2] *.[ch] refactoring: make use of the freez() wrapper
On 06/09, Ævar Arnfjörð Bjarmason wrote: > On Fri, Jun 9, 2017 at 12:12 PM, Martin Ågren wrote: > > On 9 June 2017 at 10:53, Ævar Arnfjörð Bjarmason wrote: > >> Replace occurrences of `free(p); p = NULL` with `freez(p)`. This > >> introduces no functional changes, but cuts the number of lines spent > >> on this cleanup in half. > > > > It's even better than that. ;) > > > >> 48 files changed, 97 insertions(+), 197 deletions(-) > > > > The difference is in builtin/am.c where some empty lines are removed > > in am_next(), so no need to be alarmed. > > > > The macro would be dangerous with things like "freez(ptr++)" but I > > couldn't find any such side-effects. In hindsight, I guess your commit > > message says as much since using "ptr++" for "p" would already be a > > bug. It also couldn't hurt to add a comment to the macro definition explaining that side-effect operators would be broken. > > Yes, although perhaps we should call this FREEZ() or GIT_FREEZ() > instead of freez() to make it clear that it's a macro. > > > I have no idea whether this conflicts with other topics, or any > > opinion on the best strategy for doing the conversion (all-at-once or > > "while we're here"). > > It has no conflicts with pu, so that's something, and passes all tests > with & without that merge. -- Brandon Williams
Re: [PATCH 2/2] *.[ch] refactoring: make use of the freez() wrapper
On Fri, Jun 9, 2017 at 12:12 PM, Martin Ågren wrote: > On 9 June 2017 at 10:53, Ævar Arnfjörð Bjarmason wrote: >> Replace occurrences of `free(p); p = NULL` with `freez(p)`. This >> introduces no functional changes, but cuts the number of lines spent >> on this cleanup in half. > > It's even better than that. ;) > >> 48 files changed, 97 insertions(+), 197 deletions(-) > > The difference is in builtin/am.c where some empty lines are removed > in am_next(), so no need to be alarmed. > > The macro would be dangerous with things like "freez(ptr++)" but I > couldn't find any such side-effects. In hindsight, I guess your commit > message says as much since using "ptr++" for "p" would already be a > bug. Yes, although perhaps we should call this FREEZ() or GIT_FREEZ() instead of freez() to make it clear that it's a macro. > I have no idea whether this conflicts with other topics, or any > opinion on the best strategy for doing the conversion (all-at-once or > "while we're here"). It has no conflicts with pu, so that's something, and passes all tests with & without that merge.
Re: [PATCH 2/2] *.[ch] refactoring: make use of the freez() wrapper
On 9 June 2017 at 10:53, Ævar Arnfjörð Bjarmason wrote: > Replace occurrences of `free(p); p = NULL` with `freez(p)`. This > introduces no functional changes, but cuts the number of lines spent > on this cleanup in half. It's even better than that. ;) > 48 files changed, 97 insertions(+), 197 deletions(-) The difference is in builtin/am.c where some empty lines are removed in am_next(), so no need to be alarmed. The macro would be dangerous with things like "freez(ptr++)" but I couldn't find any such side-effects. In hindsight, I guess your commit message says as much since using "ptr++" for "p" would already be a bug. I have no idea whether this conflicts with other topics, or any opinion on the best strategy for doing the conversion (all-at-once or "while we're here").
[PATCH 2/2] *.[ch] refactoring: make use of the freez() wrapper
Replace occurrences of `free(p); p = NULL` with `freez(p)`. This
introduces no functional changes, but cuts the number of lines spent
on this cleanup in half.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
alias.c | 6 ++
apply.c | 3 +--
attr.c | 6 ++
blame.c | 3 +--
branch.c | 3 +--
builtin/am.c | 18 +-
builtin/clean.c | 6 ++
builtin/config.c | 6 ++
builtin/index-pack.c | 6 ++
builtin/pack-objects.c | 12
builtin/unpack-objects.c | 3 +--
builtin/worktree.c | 6 ++
commit-slab.h| 3 +--
commit.c | 3 +--
config.c | 3 +--
credential.c | 9 +++--
diff-lib.c | 3 +--
diff.c | 6 ++
diffcore-rename.c| 6 ++
dir.c| 9 +++--
fast-import.c| 6 ++
gpg-interface.c | 15 +--
grep.c | 12
help.c | 3 +--
http-push.c | 24
http.c | 15 +--
imap-send.c | 3 +--
line-log.c | 6 ++
ll-merge.c | 3 +--
mailinfo.c | 3 +--
object.c | 3 +--
pathspec.c | 3 +--
prio-queue.c | 3 +--
read-cache.c | 6 ++
ref-filter.c | 3 +--
refs/files-backend.c | 3 +--
refs/ref-cache.c | 3 +--
remote-testsvn.c | 3 +--
rerere.c | 3 +--
sequencer.c | 3 +--
sha1-array.c | 3 +--
sha1_file.c | 3 +--
split-index.c| 3 +--
transport-helper.c | 27 +--
transport.c | 3 +--
tree-diff.c | 6 ++
tree-walk.c | 3 +--
tree.c | 3 +--
48 files changed, 97 insertions(+), 197 deletions(-)
diff --git a/alias.c b/alias.c
index 3b90397a99..99cd547b6b 100644
--- a/alias.c
+++ b/alias.c
@@ -47,8 +47,7 @@ int split_cmdline(char *cmdline, const char ***argv)
src++;
c = cmdline[src];
if (!c) {
- free(*argv);
- *argv = NULL;
+ freez(*argv);
return -SPLIT_CMDLINE_BAD_ENDING;
}
}
@@ -60,8 +59,7 @@ int split_cmdline(char *cmdline, const char ***argv)
cmdline[dst] = 0;
if (quoted) {
- free(*argv);
- *argv = NULL;
+ freez(*argv);
return -SPLIT_CMDLINE_UNCLOSED_QUOTE;
}
diff --git a/apply.c b/apply.c
index c49cef0637..f987e1e961 100644
--- a/apply.c
+++ b/apply.c
@@ -3705,8 +3705,7 @@ static int check_preimage(struct apply_state *state,
is_new:
patch->is_new = 1;
patch->is_delete = 0;
- free(patch->old_name);
- patch->old_name = NULL;
+ freez(patch->old_name);
return 0;
}
diff --git a/attr.c b/attr.c
index 7e2134471c..cd92a02e98 100644
--- a/attr.c
+++ b/attr.c
@@ -638,13 +638,11 @@ void attr_check_reset(struct attr_check *check)
void attr_check_clear(struct attr_check *check)
{
- free(check->items);
- check->items = NULL;
+ freez(check->items);
check->alloc = 0;
check->nr = 0;
- free(check->all_attrs);
- check->all_attrs = NULL;
+ freez(check->all_attrs);
check->all_attrs_nr = 0;
drop_attr_stack(&check->stack);
diff --git a/blame.c b/blame.c
index 843c845cba..dc169c 100644
--- a/blame.c
+++ b/blame.c
@@ -314,8 +314,7 @@ static void fill_origin_blob(struct diff_options *opt,
static void drop_origin_blob(struct blame_origin *o)
{
if (o->file.ptr) {
- free(o->file.ptr);
- o->file.ptr = NULL;
+ freez(o->file.ptr);
}
}
diff --git a/branch.c b/branch.c
index 985316eb76..ef2ae0b49e 100644
--- a/branch.c
+++ b/branch.c
@@ -24,8 +24,7 @@ static int find_tracked_branch(struct remote *remote, void
*priv)
} else {
free(tracking->spec.src);
if (tracking->src) {
- free(tracking->src);
- tracking->src = NULL;
+ freez(tracking->src);
}
}
tracking->spec.src = NULL;
diff --git a/builtin/am.c b/builtin/am.c
index 5ee146bfb3..f5441cc990 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -483,8 +483,7 @@ static int run_applypatch_msg_hook(struct am_state *state)
ret = run_hook_le(NULL, "applypa
