| Message ID | 359355fb4eff6d99cb1baad9b72ff96e7dcda51d.1606251358.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grep: simplify "grep defaults" handling | expand |
Martin Ågren <martin.agren@gmail.com> writes: > We have a `struct grep_opt` with our defaults which we then copy into > the caller's struct. Rather than zeroing the target struct and copying > each element one by one, just copy everything at once. This leaves the > code simpler and more maintainable. > > We don't have any ownership issues with what we're copying now and can > just greedily copy the whole thing. If and when we do need to handle > such elements (`char *`?), we must and can handle it appropriately. That is correct, but ... > This > commit doesn't really change that. ... I suspect this is not. In the original code, those who are adding a new field would notice that it is not copied over to the new instance (because they didn't add anything to grep_init() to copy the field) and at that point they must stop and think how the new field need to be copied. The structure assignment of the outer shell done in this patch means they are robbed of the opportunity to stop and think, because most of the time it "works" out of the box. I'd feel safer if we left a clue to future developers if we were to do your clean-up, perhaps like: diff --git c/grep.h w/grep.h index b5c4e223a8..388d226da3 100644 --- c/grep.h +++ w/grep.h @@ -115,6 +115,14 @@ struct grep_expr { } u; }; +/* + * grep_config() initializes one "default" instance of this type, and + * it is copied by grep_init() to be used by each individual + * invocation. When adding a new field to this structure that is + * populated from the configuration, be sure to think about ownership + * (i.e. a shallow copy may not be what you want for the type of your + * newly added field). + */ struct grep_opt { struct grep_pat *pattern_list; struct grep_pat **pattern_tail;
On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > We don't have any ownership issues with what we're copying now and can > > just greedily copy the whole thing. If and when we do need to handle > > such elements (`char *`?), we must and can handle it appropriately. > > That is correct, but ... > > > This > > commit doesn't really change that. > > ... I suspect this is not. > > In the original code, those who are adding a new field would notice > that it is not copied over to the new instance (because they didn't > add anything to grep_init() to copy the field) and at that point > they must stop and think how the new field need to be copied. > > The structure assignment of the outer shell done in this patch means > they are robbed of the opportunity to stop and think, because most > of the time it "works" out of the box. I'd feel safer if we left a > clue to future developers if we were to do your clean-up, perhaps > like: > > diff --git c/grep.h w/grep.h > index b5c4e223a8..388d226da3 100644 > --- c/grep.h > +++ w/grep.h > @@ -115,6 +115,14 @@ struct grep_expr { > } u; > }; > > +/* > + * grep_config() initializes one "default" instance of this type, and > + * it is copied by grep_init() to be used by each individual > + * invocation. When adding a new field to this structure that is > + * populated from the configuration, be sure to think about ownership > + * (i.e. a shallow copy may not be what you want for the type of your > + * newly added field). > + */ > struct grep_opt { > struct grep_pat *pattern_list; > struct grep_pat **pattern_tail; Ok, that makes sense. Maybe put it in `grep_config()` though? We can add anything we want to to this struct and initialize it from the command line. It's when we start pre-filling it in `grep_config()` that we need to think about this. What do you think? We could also do both of course to really hedge our bets... /* * The instance of grep_opt that we set up here is copied by * grep_init() to be used by each individual invocation. * When populating a new field of this structure here, * be sure to think about ownership (i.e. a shallow copy in * grep_init() may not be what you want). */ Thanks Martin
Martin Ågren <martin.agren@gmail.com> writes: > On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote: >> >> +/* >> + * grep_config() initializes one "default" instance of this type, and >> + * it is copied by grep_init() to be used by each individual >> + * invocation. When adding a new field to this structure that is >> + * populated from the configuration, be sure to think about ownership >> + * (i.e. a shallow copy may not be what you want for the type of your >> + * newly added field). >> + */ >> struct grep_opt { >> struct grep_pat *pattern_list; >> struct grep_pat **pattern_tail; > > Ok, that makes sense. Maybe put it in `grep_config()` though? We can add > anything we want to to this struct and initialize it from the command > line. It's when we start pre-filling it in `grep_config()` that we need > to think about this. What do you think? We could also do both of > course to really hedge our bets... I agree with you that it would be the most helpful to have the comment near grep_config(), as that function is what defines the design of populating the singleton to be copied by the instance used by individual invocation. > /* > * The instance of grep_opt that we set up here is copied by > * grep_init() to be used by each individual invocation. > * When populating a new field of this structure here, > * be sure to think about ownership (i.e. a shallow copy in > * grep_init() may not be what you want). > */ I find the text near the end of both my version and yours a bit unsatisfying. One thing I care about is not to mislead readers to think that the way grep_init() copies the singleton template is correct and sacred and they need to design their data structure to be compatible with the shallow copying. We'd want it to be clear that it is expected that they will deep copy the field, and release it once individual invocation is done, when they need a new field that won't work well with shallow copying. Perhaps "may not be wnat you want" is explicit enough, but I dunno. Thanks.
On Wed, 25 Nov 2020 at 08:53, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > /* > > * The instance of grep_opt that we set up here is copied by > > * grep_init() to be used by each individual invocation. > > * When populating a new field of this structure here, > > * be sure to think about ownership (i.e. a shallow copy in > > * grep_init() may not be what you want). > > */ > > I find the text near the end of both my version and yours a bit > unsatisfying. One thing I care about is not to mislead readers to > think that the way grep_init() copies the singleton template is > correct and sacred and they need to design their data structure to > be compatible with the shallow copying. We'd want it to be clear > that it is expected that they will deep copy the field, and release > it once individual invocation is done, when they need a new field > that won't work well with shallow copying. Perhaps "may not be wnat > you want" is explicit enough, but I dunno. I understand your concern. Here's what I'm considering using: /* * The instance of grep_opt that we set up here is copied by * grep_init() to be used by each individual invocation. * When populating a new field of this structure here, be * sure to think about ownership -- e.g., you might need to * override the shallow copy in grep_init() with a deep copy. */ I'm not going into the releasing of those resources, but hopefully that part can be left to the reader. Other suggestions welcome, of course. I hope to get around to rerolling this on the weekend. Martin
diff --git a/grep.c b/grep.c index 8f2009ec9f..7d740452cd 100644 --- a/grep.c +++ b/grep.c @@ -66,11 +66,6 @@ static const char *color_grep_slots[] = { [GREP_COLOR_SEP] = "separator", }; -static void color_set(char *dst, const char *color_bytes) -{ - xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes); -} - static int parse_pattern_type_arg(const char *opt, const char *arg) { if (!strcmp(arg, "default")) @@ -157,9 +152,6 @@ int grep_config(const char *var, const char *value, void *cb) */ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix) { - struct grep_opt *def = &grep_defaults; - int i; - #if defined(USE_LIBPCRE2) if (!pcre2_global_context) pcre2_global_context = pcre2_general_context_create( @@ -171,26 +163,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix pcre_free = free; #endif - memset(opt, 0, sizeof(*opt)); + *opt = grep_defaults; + opt->repo = repo; opt->prefix = prefix; opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0; opt->pattern_tail = &opt->pattern_list; opt->header_tail = &opt->header_list; - - opt->only_matching = def->only_matching; - opt->color = def->color; - opt->extended_regexp_option = def->extended_regexp_option; - opt->pattern_type_option = def->pattern_type_option; - opt->linenum = def->linenum; - opt->columnnum = def->columnnum; - opt->max_depth = def->max_depth; - opt->pathname = def->pathname; - opt->relative = def->relative; - opt->output = def->output; - - for (i = 0; i < NR_GREP_COLORS; i++) - color_set(opt->colors[i], def->colors[i]); } void grep_destroy(void)
We have a `struct grep_opt` with our defaults which we then copy into the caller's struct. Rather than zeroing the target struct and copying each element one by one, just copy everything at once. This leaves the code simpler and more maintainable. We don't have any ownership issues with what we're copying now and can just greedily copy the whole thing. If and when we do need to handle such elements (`char *`?), we must and can handle it appropriately. This commit doesn't really change that. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- grep.c | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-)