Re: [PATCH] Use ALLOC_GROW() instead of inline code
2014-02-28 4:45 GMT+08:00 Dmitry S. Dolzhenko :
> Signed-off-by: Dmitry S. Dolzhenko
> ---
> attr.c | 7 +--
> builtin/pack-objects.c | 7 +--
> bundle.c | 6 +-
> cache-tree.c | 6 +-
> commit.c | 8 ++--
> diff.c | 12 ++--
> diffcore-rename.c | 12 ++--
> dir.c | 5 +
> patch-ids.c| 5 +
> read-cache.c | 9 ++---
> reflog-walk.c | 13 +++--
> replace_object.c | 8 ++--
> 12 files changed, 19 insertions(+), 79 deletions(-)
>
I find another two files can be fixed this way.
- builtin/mktree.c
- sha1_file.c
I find it by searching the key word "alloc_nr" throughout the source code.
May be there are other places of this kind of code and you can find it
by other ways.
> diff --git a/attr.c b/attr.c
> index 8d13d70..734222d 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -338,12 +338,7 @@ static void handle_attr_line(struct attr_stack *res,
> a = parse_attr_line(line, src, lineno, macro_ok);
> if (!a)
> return;
> - if (res->alloc <= res->num_matches) {
> - res->alloc = alloc_nr(res->num_matches);
> - res->attrs = xrealloc(res->attrs,
> - sizeof(struct match_attr *) *
> - res->alloc);
> - }
> + ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
> res->attrs[res->num_matches++] = a;
> }
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 541667f..92cbce8 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1156,12 +1156,7 @@ static int check_pbase_path(unsigned hash)
> if (0 <= pos)
> return 1;
> pos = -pos - 1;
> - if (done_pbase_paths_alloc <= done_pbase_paths_num) {
> - done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
> - done_pbase_paths = xrealloc(done_pbase_paths,
> - done_pbase_paths_alloc *
> - sizeof(unsigned));
> - }
> + ALLOC_GROW(done_pbase_paths, done_pbase_paths_num + 1,
> done_pbase_paths_alloc);
> done_pbase_paths_num++;
> if (pos < done_pbase_paths_num)
> memmove(done_pbase_paths + pos + 1,
> diff --git a/bundle.c b/bundle.c
> index e99065c..1388a3e 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -14,11 +14,7 @@ static const char bundle_signature[] = "# v2 git bundle\n";
> static void add_to_ref_list(const unsigned char *sha1, const char *name,
> struct ref_list *list)
> {
> - if (list->nr + 1 >= list->alloc) {
> - list->alloc = alloc_nr(list->nr + 1);
> - list->list = xrealloc(list->list,
> - list->alloc * sizeof(list->list[0]));
> - }
> + ALLOC_GROW(list->list, list->nr + 1, list->alloc);
> memcpy(list->list[list->nr].sha1, sha1, 20);
> list->list[list->nr].name = xstrdup(name);
> list->nr++;
> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..30149d1 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -75,11 +75,7 @@ static struct cache_tree_sub *find_subtree(struct
> cache_tree *it,
> return NULL;
>
> pos = -pos-1;
> - if (it->subtree_alloc <= it->subtree_nr) {
> - it->subtree_alloc = alloc_nr(it->subtree_alloc);
> - it->down = xrealloc(it->down, it->subtree_alloc *
> - sizeof(*it->down));
> - }
> + ALLOC_GROW(it->down, it->subtree_nr + 1, it->subtree_alloc);
> it->subtree_nr++;
>
> down = xmalloc(sizeof(*down) + pathlen + 1);
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..e004314 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -147,12 +147,8 @@ int register_commit_graft(struct commit_graft *graft,
> int ignore_dups)
> return 1;
> }
> pos = -pos - 1;
> - if (commit_graft_alloc <= ++commit_graft_nr) {
> - commit_graft_alloc = alloc_nr(commit_graft_alloc);
> - commit_graft = xrealloc(commit_graft,
> - sizeof(*commit_graft) *
> - commit_graft_alloc);
> - }
> + ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
> + commit_graft_nr++;
> if (pos < commit_graft_nr)
> memmove(commit_graft + pos + 1,
> commit_graft + pos,
> diff --git a/diff.c b/diff.c
> index 8e4a6a9..f5f0fd1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct
> diffstat_t *diffstat,
> {
> struct diffstat_file *x;
> x = xcalloc(sizeof (*x), 1);
> - if (diffstat->nr == diffstat->alloc) {
Re: [PATCH] Use ALLOC_GROW() instead of inline code
"Dmitry S. Dolzhenko" writes:
> diff --git a/dir.c b/dir.c
> index b35b633..72f6e2a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1329,13 +1329,10 @@ static struct path_simplify *create_simplify(const
> char **pathspec)
>
> for (nr = 0 ; ; nr++) {
> const char *match;
> - if (nr >= alloc) {
> - alloc = alloc_nr(alloc);
> - simplify = xrealloc(simplify, alloc *
> sizeof(*simplify));
> - }
> match = *pathspec++;
> if (!match)
> break;
> + ALLOC_GROW(simplify, nr + 1, alloc);
> simplify[nr].path = match;
> simplify[nr].len = simple_length(match);
> }
What follows the post-context of this hunk is a NULL termination of
the array:
simplify[nr].path = NULL;
simplify[nr].len = 0;
If the first element in pathspec[] were NULL, we set nr to 0, break
the loop without calling ALLOC_GROW() even once, and try to NULL
terminate simplify[] array after the loop.
Don't we try to store to an unallocated piece of memory with this
change?
> diff --git a/read-cache.c b/read-cache.c
> index 33dd676..e585541 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1466,8 +1462,7 @@ int read_index_from(struct index_state *istate, const
> char *path)
>
> istate->version = ntohl(hdr->hdr_version);
> istate->cache_nr = ntohl(hdr->hdr_entries);
> - istate->cache_alloc = alloc_nr(istate->cache_nr);
> - istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
> + ALLOC_GROW(istate->cache, istate->cache_nr, istate->cache_alloc);
This being the initial allocation, not growing reallocation, use of
ALLOC_GROW() looks somewhat strange. I know that an realloc from
NULL ends up being the same as calloc(), but still.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use ALLOC_GROW() instead of inline code
Dmitry,
That's cool; I never imagined there would be so many sites that could be
cleaned up in this way.
In my opinion, it would be preferable for this patch to be broken into
multiple commits, one for each site (or each file, if a file has
multiple sites that are logically related). That would make it easier
to review the patches and easier to bisect if we later find a problem.
Please make sure that you note if there are any sites where the
rewritten code doesn't have exactly the same semantics as the original
(I don't know if there are sites like this, but if there are...). That
helps reviewers focus on the changes that might be "controversial".
[Please leave the other microprojects for other students (I just wrote
an email on this topic).]
Thanks,
Michael
On 02/27/2014 09:45 PM, Dmitry S. Dolzhenko wrote:
> Signed-off-by: Dmitry S. Dolzhenko
> ---
> attr.c | 7 +--
> builtin/pack-objects.c | 7 +--
> bundle.c | 6 +-
> cache-tree.c | 6 +-
> commit.c | 8 ++--
> diff.c | 12 ++--
> diffcore-rename.c | 12 ++--
> dir.c | 5 +
> patch-ids.c| 5 +
> read-cache.c | 9 ++---
> reflog-walk.c | 13 +++--
> replace_object.c | 8 ++--
> 12 files changed, 19 insertions(+), 79 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 8d13d70..734222d 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -338,12 +338,7 @@ static void handle_attr_line(struct attr_stack *res,
> a = parse_attr_line(line, src, lineno, macro_ok);
> if (!a)
> return;
> - if (res->alloc <= res->num_matches) {
> - res->alloc = alloc_nr(res->num_matches);
> - res->attrs = xrealloc(res->attrs,
> - sizeof(struct match_attr *) *
> - res->alloc);
> - }
> + ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
> res->attrs[res->num_matches++] = a;
> }
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 541667f..92cbce8 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1156,12 +1156,7 @@ static int check_pbase_path(unsigned hash)
> if (0 <= pos)
> return 1;
> pos = -pos - 1;
> - if (done_pbase_paths_alloc <= done_pbase_paths_num) {
> - done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
> - done_pbase_paths = xrealloc(done_pbase_paths,
> - done_pbase_paths_alloc *
> - sizeof(unsigned));
> - }
> + ALLOC_GROW(done_pbase_paths, done_pbase_paths_num + 1,
> done_pbase_paths_alloc);
> done_pbase_paths_num++;
> if (pos < done_pbase_paths_num)
> memmove(done_pbase_paths + pos + 1,
> diff --git a/bundle.c b/bundle.c
> index e99065c..1388a3e 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -14,11 +14,7 @@ static const char bundle_signature[] = "# v2 git bundle\n";
> static void add_to_ref_list(const unsigned char *sha1, const char *name,
> struct ref_list *list)
> {
> - if (list->nr + 1 >= list->alloc) {
> - list->alloc = alloc_nr(list->nr + 1);
> - list->list = xrealloc(list->list,
> - list->alloc * sizeof(list->list[0]));
> - }
> + ALLOC_GROW(list->list, list->nr + 1, list->alloc);
> memcpy(list->list[list->nr].sha1, sha1, 20);
> list->list[list->nr].name = xstrdup(name);
> list->nr++;
> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..30149d1 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -75,11 +75,7 @@ static struct cache_tree_sub *find_subtree(struct
> cache_tree *it,
> return NULL;
>
> pos = -pos-1;
> - if (it->subtree_alloc <= it->subtree_nr) {
> - it->subtree_alloc = alloc_nr(it->subtree_alloc);
> - it->down = xrealloc(it->down, it->subtree_alloc *
> - sizeof(*it->down));
> - }
> + ALLOC_GROW(it->down, it->subtree_nr + 1, it->subtree_alloc);
> it->subtree_nr++;
>
> down = xmalloc(sizeof(*down) + pathlen + 1);
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..e004314 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -147,12 +147,8 @@ int register_commit_graft(struct commit_graft *graft,
> int ignore_dups)
> return 1;
> }
> pos = -pos - 1;
> - if (commit_graft_alloc <= ++commit_graft_nr) {
> - commit_graft_alloc = alloc_nr(commit_graft_alloc);
> - commit_graft = xrealloc(commit_graft,
> - sizeof(*commit_graft) *
> - commit_graft_alloc);
> - }
> + ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
> + commit_graft_nr++;
> if (pos < comm
