[2/5] streaming.c: remove enum/function/vtbl indirection

Commit Message

Ævar Arnfjörð Bjarmason May 5, 2021, 12:33 p.m. UTC

Remove the indirection of discovering a function pointer to use via an
enum and virtual table. This refactors code added in
46bf043807c (streaming: a new API to read from the object store,
2011-05-11).

We can instead simply return an "open_istream_fn" for use from the
"istream_source()" selector function directly. This allows us to get
rid of the "incore", "loose" and "pack_non_delta" enum
variables. We'll return the functions instead.

The "stream_error" variable in that enum can likewise go in favor of
returning NULL, which is what the open_istream() was doing when it got
that value anyway.

We can thus remove the entire enum, and the "open_istream_tbl" virtual
table that (indirectly) referenced it.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 streaming.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

Comments

Jeff King May 5, 2021, 1:42 p.m. UTC | #1

On Wed, May 05, 2021 at 02:33:29PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Remove the indirection of discovering a function pointer to use via an
> enum and virtual table. This refactors code added in
> 46bf043807c (streaming: a new API to read from the object store,
> 2011-05-11).
> 
> We can instead simply return an "open_istream_fn" for use from the
> "istream_source()" selector function directly. This allows us to get
> rid of the "incore", "loose" and "pack_non_delta" enum
> variables. We'll return the functions instead.
> 
> The "stream_error" variable in that enum can likewise go in favor of
> returning NULL, which is what the open_istream() was doing when it got
> that value anyway.
> 
> We can thus remove the entire enum, and the "open_istream_tbl" virtual
> table that (indirectly) referenced it.

Yeah, I think this is simpler. The value of the vtable was that we might
have added more functions to it, but we haven't done so over the course
of the last 10 years. And I have trouble imagining for what purpose we
would. So it seems like unnecessary complexity.

-Peff

Junio C Hamano May 6, 2021, 12:14 a.m. UTC | #2

Jeff King <[e-mail]> writes:

>> We can thus remove the entire enum, and the "open_istream_tbl" virtual
>> table that (indirectly) referenced it.
>
> Yeah, I think this is simpler. The value of the vtable was that we might
> have added more functions to it, but we haven't done so over the course
> of the last 10 years. And I have trouble imagining for what purpose we
> would. So it seems like unnecessary complexity.

The code hasn't changed all that much since it was written, so
another equally plausible way forward is not to touch it at all ;-),
but I am fine with this simplification, too.

Patch

diff --git a/streaming.c b/streaming.c
index b0d439e47a7..628369519b9 100644
--- a/streaming.c
+++ b/streaming.c
@@ -8,13 +8,6 @@ 
 #include "replace-object.h"
 #include "packfile.h"
 
-enum input_source {
-	stream_error = -1,
-	incore = 0,
-	loose = 1,
-	pack_non_delta = 2
-};
-
 typedef int (*open_istream_fn)(struct git_istream *,
 			       struct repository *,
 			       struct object_info *,
@@ -424,16 +417,10 @@  static open_method_decl(incore)
  * static helpers variables and functions for users of streaming interface
  *****************************************************************************/
 
-static open_istream_fn open_istream_tbl[] = {
-	open_istream_incore,
-	open_istream_loose,
-	open_istream_pack_non_delta,
-};
-
-static enum input_source istream_source(struct repository *r,
-					const struct object_id *oid,
-					enum object_type *type,
-					struct object_info *oi)
+static open_istream_fn istream_source(struct repository *r,
+				      const struct object_id *oid,
+				      enum object_type *type,
+				      struct object_info *oi)
 {
 	unsigned long size;
 	int status;
@@ -442,21 +429,20 @@  static enum input_source istream_source(struct repository *r,
 	oi->sizep = &size;
 	status = oid_object_info_extended(r, oid, oi, 0);
 	if (status < 0)
-		return stream_error;
+		return NULL;
 
 	switch (oi->whence) {
 	case OI_LOOSE:
-		return loose;
+		return open_istream_loose;
 	case OI_PACKED:
 		if (!oi->u.packed.is_delta && big_file_threshold < size)
-			return pack_non_delta;
+			return open_istream_pack_non_delta;
 		/* fallthru */
 	default:
-		return incore;
+		return open_istream_incore;
 	}
 }
 
-
 /****************************************************************
  * Users of streaming interface
  ****************************************************************/
@@ -482,13 +468,13 @@  struct git_istream *open_istream(struct repository *r,
 	struct git_istream *st;
 	struct object_info oi = OBJECT_INFO_INIT;
 	const struct object_id *real = lookup_replace_object(r, oid);
-	enum input_source src = istream_source(r, real, type, &oi);
+	open_istream_fn open_fn = istream_source(r, real, type, &oi);
 
-	if (src < 0)
+	if (!open_fn)
 		return NULL;
 
 	st = xmalloc(sizeof(*st));
-	if (open_istream_tbl[src](st, r, &oi, real, type)) {
+	if (open_fn(st, r, &oi, real, type)) {
 		if (open_istream_incore(st, r, &oi, real, type)) {
 			free(st);
 			return NULL;