| Message ID | d6e43bc540bc682bb46d54e579a7101d0d2c462d.1605972564.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | grep: retire `init_grep_defaults()` | expand |
On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote: > The previous commit left us with only one user of the one-line wrapper > `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing > all the entries in one array into another array of the same type. We > might as well just `memcpy()` everything into place. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > Cc-ing Peff, who initially introduced this helper. After having inlined > the function into the for loop, it seemed better to just copy the whole > array. Happy to hear arguments against. No, this is way better than the existing code. I introduced it to get away from strcpy(), but this is better still. But... > Come to think of it, I suppose we could copy the whole struct and not > just the color array. Hmmm... Yes, this seems even better. If our goal is just to start our new grep_opt the same as grep_defaults, then a single-line struct copy (whether through assignment or memcpy) is even clearer and more maintainable. -Peff
On Sat, 21 Nov 2020 at 21:23, Jeff King <peff@peff.net> wrote: > > On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote: >> > Cc-ing Peff, who initially introduced this helper. After having inlined > > the function into the for loop, it seemed better to just copy the whole > > array. Happy to hear arguments against. > > No, this is way better than the existing code. I introduced it to get > away from strcpy(), but this is better still. But... > > > Come to think of it, I suppose we could copy the whole struct and not > > just the color array. Hmmm... > > Yes, this seems even better. If our goal is just to start our new > grep_opt the same as grep_defaults, then a single-line struct copy > (whether through assignment or memcpy) is even clearer and more > maintainable. Ok, thanks for the encouraging words. I couldn't keep myself from thinking that we're doing this for some weird ... performance reason? Thanks for taking me out of that thought. I'll hold off for a while in case there's more feedback, then look into replacing this patch with a more aggressive copy of the whole struct. Martin
Jeff King <peff@peff.net> writes: > On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote: > >> The previous commit left us with only one user of the one-line wrapper >> `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing >> all the entries in one array into another array of the same type. We >> might as well just `memcpy()` everything into place. >> >> Signed-off-by: Martin Ågren <martin.agren@gmail.com> >> --- >> Cc-ing Peff, who initially introduced this helper. After having inlined >> the function into the for loop, it seemed better to just copy the whole >> array. Happy to hear arguments against. > > No, this is way better than the existing code. I introduced it to get > away from strcpy(), but this is better still. But... Yes, the copy in this patch looks eminently sensible. >> Come to think of it, I suppose we could copy the whole struct and not >> just the color array. Hmmm... > > Yes, this seems even better. If our goal is just to start our new > grep_opt the same as grep_defaults, then a single-line struct copy > (whether through assignment or memcpy) is even clearer and more > maintainable. ... until such a time when typeof(grep_defaults) gains a field with a non-const pointer value that we'd rather not to share amongst instances of the type, at which point it no longer is clear win from maintainability's point of view.
On Sat, Nov 21, 2020 at 02:46:14PM -0800, Junio C Hamano wrote: > >> Come to think of it, I suppose we could copy the whole struct and not > >> just the color array. Hmmm... > > > > Yes, this seems even better. If our goal is just to start our new > > grep_opt the same as grep_defaults, then a single-line struct copy > > (whether through assignment or memcpy) is even clearer and more > > maintainable. > > ... until such a time when typeof(grep_defaults) gains a field with > a non-const pointer value that we'd rather not to share amongst > instances of the type, at which point it no longer is clear win from > maintainability's point of view. I don't think we are any worse off. Either way we need to add a special-case deep copy to the function (which should definitely happen in only one place; we do not want bare struct copies sprinkled around the code). -Peff
diff --git a/grep.c b/grep.c index 8f2009ec9f..9597cec67e 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")) @@ -158,7 +153,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) @@ -189,8 +183,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix opt->relative = def->relative; opt->output = def->output; - for (i = 0; i < NR_GREP_COLORS; i++) - color_set(opt->colors[i], def->colors[i]); + memcpy(opt->colors, def->colors, sizeof(def->colors)); } void grep_destroy(void)
The previous commit left us with only one user of the one-line wrapper `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing all the entries in one array into another array of the same type. We might as well just `memcpy()` everything into place. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- Cc-ing Peff, who initially introduced this helper. After having inlined the function into the for loop, it seemed better to just copy the whole array. Happy to hear arguments against. Come to think of it, I suppose we could copy the whole struct and not just the color array. Hmmm... grep.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)