[v2,3/8] grep: typesafe versions of grep_source_init

Commit Message

Jonathan Tan Aug. 13, 2021, 9:05 p.m. UTC

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 <[e-mail]>
---
 builtin/grep.c |  4 ++--
 grep.c         | 46 ++++++++++++++++++++++++++++------------------
 grep.h         |  7 ++++---
 3 files changed, 34 insertions(+), 23 deletions(-)

Comments

Matheus Tavares Aug. 16, 2021, 3:06 p.m. UTC | #1

On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan <[e-mail]> wrote:
>
> diff --git a/grep.c b/grep.c
> index 424a39591b..8a8105c2eb 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1840,28 +1850,28 @@ 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);

At first, it seemed a bit odd to me that we use
`xstrdup_or_null(path)` first but `xtsrdup(path)` later. But, I saw
that we already did this before this patch, and that the second call
is fine as `path` is required to be non-NULL in this case (i.e.
GREP_SOURCE_FILE), because we need it for grep_source_load_file()
later. (Also, the only caller is correctly passing a non-NULL buffer,
so all is good.)

Patch

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..8a8105c2eb 100644
--- a/grep.c
+++ b/grep.c
@@ -1825,14 +1825,24 @@  int grep_source(struct grep_opt *opt, struct grep_source *gs)
 	return grep_source_1(opt, gs, 0);
 }
 
+static void grep_source_init_buf(struct grep_source *gs, char *buf,
+				 unsigned long size)
+{
+	gs->type = GREP_SOURCE_BUF;
+	gs->name = NULL;
+	gs->path = NULL;
+	gs->buf = buf;
+	gs->size = size;
+	gs->driver = NULL;
+	gs->identifier = NULL;
+}
+
 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);
-	gs.buf = buf;
-	gs.size = size;
+	grep_source_init_buf(&gs, buf, size);
 
 	r = grep_source(opt, &gs);
 
@@ -1840,28 +1850,28 @@  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_clear(struct grep_source *gs)
diff --git a/grep.h b/grep.h
index 72f82b1e30..480b3f5bba 100644
--- a/grep.h
+++ b/grep.h
@@ -195,9 +195,10 @@  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_clear_data(struct grep_source *gs);
 void grep_source_clear(struct grep_source *gs);
 void grep_source_load_driver(struct grep_source *gs,