| Message ID | e5e6a0dc1ef59b2ab419816e463814d93115e7f6.1628618950.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | In grep, no adding submodule ODB as alternates | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > grep_source_init() can create "struct grep_source" objects and, > depending on the value of the type passed, some void-pointer parameters have > different meanings. Because one of these types (GREP_SOURCE_OID) will > require an additional parameter in a subsequent patch, take the > opportunity to increase clarity and type safety by replacing this > function with individual functions for each type. Nice clean-up.
On Tue, Aug 10, 2021 at 11:28:41AM -0700, Jonathan Tan wrote: > > grep_source_init() can create "struct grep_source" objects and, > depending on the value of the type passed, some void-pointer parameters have > different meanings. Because one of these types (GREP_SOURCE_OID) will > require an additional parameter in a subsequent patch, take the > opportunity to increase clarity and type safety by replacing this > function with individual functions for each type. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Like Junio said, it is very neat. Reviewed-by: Emily Shaffer <emilyshaffer@google.com>
On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > diff --git a/grep.c b/grep.c > index 424a39591b..ba3711dc56 100644 > --- a/grep.c > +++ b/grep.c > @@ -1830,7 +1830,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size) > struct grep_source gs; > int r; > > - grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL); > + grep_source_init_buf(&gs); > gs.buf = buf; > gs.size = size; Small nit: perhaps `grep_source_init_buf()` could take `buf` and `size` too, so that all the fields get initialized by the same function.
On Wed, Aug 11, 2021 at 02:42:20PM -0700, Emily Shaffer wrote: > On Tue, Aug 10, 2021 at 11:28:41AM -0700, Jonathan Tan wrote: > > > > grep_source_init() can create "struct grep_source" objects and, > > depending on the value of the type passed, some void-pointer parameters have > > different meanings. Because one of these types (GREP_SOURCE_OID) will > > require an additional parameter in a subsequent patch, take the > > opportunity to increase clarity and type safety by replacing this > > function with individual functions for each type. > > > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > Like Junio said, it is very neat. [Sorry for piggy-backing, I have already deleted the original mail :( ] Just a quick note: grep_source_init_buf() is only called from grep.c:1833, before its definition at grep.c:1869, so it could be marked as static (as things stand). Do you anticipate any future callers from outside of grep.c? (after removing the declaration from grep.h, you would need to add a forward declaration or, better, move the definition to before the call (or the call (and grep_buffer()) after the definition)). ATB, Ramsay Jones
Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan <jonathantanmy@google.com> wrote: >> >> diff --git a/grep.c b/grep.c >> index 424a39591b..ba3711dc56 100644 >> --- a/grep.c >> +++ b/grep.c >> @@ -1830,7 +1830,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size) >> struct grep_source gs; >> int r; >> >> - grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL); >> + grep_source_init_buf(&gs); >> gs.buf = buf; >> gs.size = size; > > Small nit: perhaps `grep_source_init_buf()` could take `buf` and > `size` too, so that all the fields get initialized by the same > function. Sounds sensible. Thanks.
> Just a quick note: grep_source_init_buf() is only called from > grep.c:1833, before its definition at grep.c:1869, so it could be marked > as static (as things stand). Do you anticipate any future callers from > outside of grep.c? (after removing the declaration from grep.h, you > would need to add a forward declaration or, better, move the definition > to before the call (or the call (and grep_buffer()) after the definition)). No, I do not - thanks for the note, and I'll make it static in the next reroll.
> > Small nit: perhaps `grep_source_init_buf()` could take `buf` and > > `size` too, so that all the fields get initialized by the same > > function. > > Sounds sensible. Thanks. Makes sense - I'll do this.
diff --git a/builtin/grep.c b/builtin/grep.c index 87bcb934a2..e454335e9d 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -333,7 +333,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid, struct grep_source gs; grep_source_name(opt, filename, tree_name_len, &pathbuf); - grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid); + grep_source_init_oid(&gs, pathbuf.buf, path, oid); strbuf_release(&pathbuf); if (num_threads > 1) { @@ -359,7 +359,7 @@ static int grep_file(struct grep_opt *opt, const char *filename) struct grep_source gs; grep_source_name(opt, filename, 0, &buf); - grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename); + grep_source_init_file(&gs, buf.buf, filename); strbuf_release(&buf); if (num_threads > 1) { diff --git a/grep.c b/grep.c index 424a39591b..ba3711dc56 100644 --- a/grep.c +++ b/grep.c @@ -1830,7 +1830,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size) struct grep_source gs; int r; - grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL); + grep_source_init_buf(&gs); gs.buf = buf; gs.size = size; @@ -1840,28 +1840,39 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size) return r; } -void grep_source_init(struct grep_source *gs, enum grep_source_type type, - const char *name, const char *path, - const void *identifier) +void grep_source_init_file(struct grep_source *gs, const char *name, + const char *path) { - gs->type = type; + gs->type = GREP_SOURCE_FILE; gs->name = xstrdup_or_null(name); gs->path = xstrdup_or_null(path); gs->buf = NULL; gs->size = 0; gs->driver = NULL; + gs->identifier = xstrdup(path); +} - switch (type) { - case GREP_SOURCE_FILE: - gs->identifier = xstrdup(identifier); - break; - case GREP_SOURCE_OID: - gs->identifier = oiddup(identifier); - break; - case GREP_SOURCE_BUF: - gs->identifier = NULL; - break; - } +void grep_source_init_oid(struct grep_source *gs, const char *name, + const char *path, const struct object_id *oid) +{ + gs->type = GREP_SOURCE_OID; + gs->name = xstrdup_or_null(name); + gs->path = xstrdup_or_null(path); + gs->buf = NULL; + gs->size = 0; + gs->driver = NULL; + gs->identifier = oiddup(oid); +} + +void grep_source_init_buf(struct grep_source *gs) +{ + gs->type = GREP_SOURCE_BUF; + gs->name = NULL; + gs->path = NULL; + gs->buf = NULL; + gs->size = 0; + gs->driver = NULL; + gs->identifier = NULL; } void grep_source_clear(struct grep_source *gs) diff --git a/grep.h b/grep.h index 72f82b1e30..f4a3090f1c 100644 --- a/grep.h +++ b/grep.h @@ -195,9 +195,11 @@ struct grep_source { struct userdiff_driver *driver; }; -void grep_source_init(struct grep_source *gs, enum grep_source_type type, - const char *name, const char *path, - const void *identifier); +void grep_source_init_file(struct grep_source *gs, const char *name, + const char *path); +void grep_source_init_oid(struct grep_source *gs, const char *name, + const char *path, const struct object_id *oid); +void grep_source_init_buf(struct grep_source *gs); void grep_source_clear_data(struct grep_source *gs); void grep_source_clear(struct grep_source *gs); void grep_source_load_driver(struct grep_source *gs,
grep_source_init() can create "struct grep_source" objects and, depending on the value of the type passed, some void-pointer parameters have different meanings. Because one of these types (GREP_SOURCE_OID) will require an additional parameter in a subsequent patch, take the opportunity to increase clarity and type safety by replacing this function with individual functions for each type. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- builtin/grep.c | 4 ++-- grep.c | 43 +++++++++++++++++++++++++++---------------- grep.h | 8 +++++--- 3 files changed, 34 insertions(+), 21 deletions(-)