| Message ID | 20190601003603.90794-9-matvore@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Filter combination | expand |
On Fri, May 31, 2019 at 5:40 PM Matthew DeVore <matvore@google.com> wrote: > > Introduce a new macro ALLOC_GROW_BY which automatically zeros the added > array elements and takes care of updating the nr value. Use the macro in > code introduced earlier in this patchset. > > Signed-off-by: Matthew DeVore <matvore@google.com> > --- > cache.h | 22 ++++++++++++++++++++++ > list-objects-filter-options.c | 17 +++++++---------- > 2 files changed, 29 insertions(+), 10 deletions(-) > > diff --git a/cache.h b/cache.h > index fa8ede9a2d..847fbdeff0 100644 > --- a/cache.h > +++ b/cache.h > @@ -652,33 +652,55 @@ int init_db(const char *git_dir, const char *real_git_dir, > void sanitize_stdfds(void); > int daemonize(void); > > #define alloc_nr(x) (((x)+16)*3/2) > > /* > * Realloc the buffer pointed at by variable 'x' so that it can hold > * at least 'nr' entries; the number of entries currently allocated > * is 'alloc', using the standard growing factor alloc_nr() macro. > * > + * Consider using ALLOC_GROW_BY instead of ALLOC_GROW as it has some > + * added niceties. > + * > * DO NOT USE any expression with side-effect for 'x', 'nr', or 'alloc'. > */ > #define ALLOC_GROW(x, nr, alloc) \ > do { \ > if ((nr) > alloc) { \ > if (alloc_nr(alloc) < (nr)) \ > alloc = (nr); \ > else \ > alloc = alloc_nr(alloc); \ > REALLOC_ARRAY(x, alloc); \ > } \ > } while (0) > > +/* > + * Similar to ALLOC_GROW but handles updating of the nr value and > + * zeroing the bytes of the newly-grown array elements. > + * > + * DO NOT USE any expression with side-effect for any of the > + * arguments. > + */ Since ALLOC_GROW already doesn't handle this safely, there isn't necessarily a reason to fix it, but you could read the macro values into temporary variables inside the do { } while(0) loop in order to avoid the multiple-expansion side effect issues... Thanks, Jake > +#define ALLOC_GROW_BY(x, nr, increase, alloc) \ > + do { \ > + if (increase) { \ > + size_t new_nr = nr + (increase); \ > + if (new_nr < nr) \ > + BUG("negative growth in ALLOC_GROW_BY"); \ > + ALLOC_GROW(x, new_nr, alloc); \ > + memset((x) + nr, 0, sizeof(*(x)) * (increase)); \ > + nr = new_nr; \ > + } \ > + } while (0) > + > /* Initialize and use the cache information */ > struct lock_file; > void preload_index(struct index_state *index, > const struct pathspec *pathspec, > unsigned int refresh_flags); > int do_read_index(struct index_state *istate, const char *path, > int must_exist); /* for testting only! */ > int read_index_from(struct index_state *, const char *path, > const char *gitdir); > int is_index_unborn(struct index_state *); > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c > index 5e98e4a309..d8abe6cfcf 100644 > --- a/list-objects-filter-options.c > +++ b/list-objects-filter-options.c > @@ -142,26 +142,24 @@ static int has_reserved_character( > } > > return 0; > } > > static int parse_combine_subfilter( > struct list_objects_filter_options *filter_options, > struct strbuf *subspec, > struct strbuf *errbuf) > { > - size_t new_index = filter_options->sub_nr++; > + size_t new_index = filter_options->sub_nr; > > - ALLOC_GROW(filter_options->sub, filter_options->sub_nr, > - filter_options->sub_alloc); > - memset(&filter_options->sub[new_index], 0, > - sizeof(*filter_options->sub)); > + ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, > + filter_options->sub_alloc); > > return has_reserved_character(subspec, errbuf) || > url_decode(subspec, errbuf) || > gently_parse_list_objects_filter( > &filter_options->sub[new_index], subspec->buf, errbuf); > } > > static int parse_combine_filter( > struct list_objects_filter_options *filter_options, > const char *arg, > @@ -273,27 +271,26 @@ int parse_list_objects_filter( > /* > * Make filter_options an LOFC_COMBINE spec so we can trivially > * add subspecs to it. > */ > transform_to_combine_type(filter_options); > > strbuf_addstr(&filter_options->filter_spec, "+"); > add_url_encoded(&filter_options->filter_spec, arg); > trace_printf("Generated composite filter-spec: %s\n", > filter_options->filter_spec.buf); > - ALLOC_GROW(filter_options->sub, filter_options->sub_nr + 1, > - filter_options->sub_alloc); > - filter_options = &filter_options->sub[filter_options->sub_nr++]; > - memset(filter_options, 0, sizeof(*filter_options)); > + ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, > + filter_options->sub_alloc); > > parse_error = gently_parse_list_objects_filter( > - filter_options, arg, &errbuf); > + &filter_options->sub[filter_options->sub_nr - 1], arg, > + &errbuf); > } > if (parse_error) > die("%s", errbuf.buf); > return 0; > } > > int opt_parse_list_objects_filter(const struct option *opt, > const char *arg, int unset) > { > struct list_objects_filter_options *filter_options = opt->value; > -- > 2.17.1 >
On Mon, Jun 03, 2019 at 03:07:40PM -0700, Jacob Keller wrote: > > +/* > > + * Similar to ALLOC_GROW but handles updating of the nr value and > > + * zeroing the bytes of the newly-grown array elements. > > + * > > + * DO NOT USE any expression with side-effect for any of the > > + * arguments. > > + */ > > Since ALLOC_GROW already doesn't handle this safely, there isn't > necessarily a reason to fix it, but you could read the macro values > into temporary variables inside the do { } while(0) loop in order to > avoid the multiple-expansion side effect issues... For x I don't think that's possible since we don't know the pointer type. For nr and alloc it doesn't make sense since they're being assigned to. For `increase` I could try this: size_t ALLOC_GROW_BY__increase = (increase); but I'm not sure how well this works when `increase` is a signed type. This seemed sufficiently pitfall-y that I didn't attempt it. Relatedly, I was thinking something like this would be nice, if anyone has time for such a refactor: struct growth_info { size_t nr, alloc; } And use that to replace individual "size_t foo_nr, foo_alloc" And make ALLOC_GROW_BY use it. I think a bulk, maybe even most, ALLOC_GROW invocations can be changed to ALLOC_GROW_BY.
On Mon, Jun 3, 2019 at 3:39 PM Matthew DeVore <matvore@comcast.net> wrote: > > On Mon, Jun 03, 2019 at 03:07:40PM -0700, Jacob Keller wrote: > > > +/* > > > + * Similar to ALLOC_GROW but handles updating of the nr value and > > > + * zeroing the bytes of the newly-grown array elements. > > > + * > > > + * DO NOT USE any expression with side-effect for any of the > > > + * arguments. > > > + */ > > > > Since ALLOC_GROW already doesn't handle this safely, there isn't > > necessarily a reason to fix it, but you could read the macro values > > into temporary variables inside the do { } while(0) loop in order to > > avoid the multiple-expansion side effect issues... > > For x I don't think that's possible since we don't know the pointer type. For > nr and alloc it doesn't make sense since they're being assigned to. For > `increase` I could try this: > Ah.. you could do the compiler typeof extensions, but I guess we probably don't wanna rely on that. > size_t ALLOC_GROW_BY__increase = (increase); > > but I'm not sure how well this works when `increase` is a signed type. This > seemed sufficiently pitfall-y that I didn't attempt it. Ok that makes sense. Regards, Jake
diff --git a/cache.h b/cache.h index fa8ede9a2d..847fbdeff0 100644 --- a/cache.h +++ b/cache.h @@ -652,33 +652,55 @@ int init_db(const char *git_dir, const char *real_git_dir, void sanitize_stdfds(void); int daemonize(void); #define alloc_nr(x) (((x)+16)*3/2) /* * Realloc the buffer pointed at by variable 'x' so that it can hold * at least 'nr' entries; the number of entries currently allocated * is 'alloc', using the standard growing factor alloc_nr() macro. * + * Consider using ALLOC_GROW_BY instead of ALLOC_GROW as it has some + * added niceties. + * * DO NOT USE any expression with side-effect for 'x', 'nr', or 'alloc'. */ #define ALLOC_GROW(x, nr, alloc) \ do { \ if ((nr) > alloc) { \ if (alloc_nr(alloc) < (nr)) \ alloc = (nr); \ else \ alloc = alloc_nr(alloc); \ REALLOC_ARRAY(x, alloc); \ } \ } while (0) +/* + * Similar to ALLOC_GROW but handles updating of the nr value and + * zeroing the bytes of the newly-grown array elements. + * + * DO NOT USE any expression with side-effect for any of the + * arguments. + */ +#define ALLOC_GROW_BY(x, nr, increase, alloc) \ + do { \ + if (increase) { \ + size_t new_nr = nr + (increase); \ + if (new_nr < nr) \ + BUG("negative growth in ALLOC_GROW_BY"); \ + ALLOC_GROW(x, new_nr, alloc); \ + memset((x) + nr, 0, sizeof(*(x)) * (increase)); \ + nr = new_nr; \ + } \ + } while (0) + /* Initialize and use the cache information */ struct lock_file; void preload_index(struct index_state *index, const struct pathspec *pathspec, unsigned int refresh_flags); int do_read_index(struct index_state *istate, const char *path, int must_exist); /* for testting only! */ int read_index_from(struct index_state *, const char *path, const char *gitdir); int is_index_unborn(struct index_state *); diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 5e98e4a309..d8abe6cfcf 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -142,26 +142,24 @@ static int has_reserved_character( } return 0; } static int parse_combine_subfilter( struct list_objects_filter_options *filter_options, struct strbuf *subspec, struct strbuf *errbuf) { - size_t new_index = filter_options->sub_nr++; + size_t new_index = filter_options->sub_nr; - ALLOC_GROW(filter_options->sub, filter_options->sub_nr, - filter_options->sub_alloc); - memset(&filter_options->sub[new_index], 0, - sizeof(*filter_options->sub)); + ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, + filter_options->sub_alloc); return has_reserved_character(subspec, errbuf) || url_decode(subspec, errbuf) || gently_parse_list_objects_filter( &filter_options->sub[new_index], subspec->buf, errbuf); } static int parse_combine_filter( struct list_objects_filter_options *filter_options, const char *arg, @@ -273,27 +271,26 @@ int parse_list_objects_filter( /* * Make filter_options an LOFC_COMBINE spec so we can trivially * add subspecs to it. */ transform_to_combine_type(filter_options); strbuf_addstr(&filter_options->filter_spec, "+"); add_url_encoded(&filter_options->filter_spec, arg); trace_printf("Generated composite filter-spec: %s\n", filter_options->filter_spec.buf); - ALLOC_GROW(filter_options->sub, filter_options->sub_nr + 1, - filter_options->sub_alloc); - filter_options = &filter_options->sub[filter_options->sub_nr++]; - memset(filter_options, 0, sizeof(*filter_options)); + ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, + filter_options->sub_alloc); parse_error = gently_parse_list_objects_filter( - filter_options, arg, &errbuf); + &filter_options->sub[filter_options->sub_nr - 1], arg, + &errbuf); } if (parse_error) die("%s", errbuf.buf); return 0; } int opt_parse_list_objects_filter(const struct option *opt, const char *arg, int unset) { struct list_objects_filter_options *filter_options = opt->value;
Introduce a new macro ALLOC_GROW_BY which automatically zeros the added array elements and takes care of updating the nr value. Use the macro in code introduced earlier in this patchset. Signed-off-by: Matthew DeVore <matvore@google.com> --- cache.h | 22 ++++++++++++++++++++++ list-objects-filter-options.c | 17 +++++++---------- 2 files changed, 29 insertions(+), 10 deletions(-)