Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`

From: Jeff King <peff@peff.net>
Date: Oct 25, 2023, 07:37 UTC
Message-ID: <20231025073736.GB2145145@coredump.intra.peff.net>
In-Reply-To: <696aa027e46ddec310812fad2d4b12082447d925.1698101088.git.me@ttaylorr.com>

On Mon, Oct 23, 2023 at 06:44:56PM -0400, Taylor Blau wrote:

Show 13 quoted lines

> +struct bulk_checkin_source {
> +	off_t (*read)(struct bulk_checkin_source *, void *, size_t);
> +	off_t (*seek)(struct bulk_checkin_source *, off_t);
> +
> +	union {
> +		struct {
> +			int fd;
> +		} from_fd;
> +	} data;
> +
> +	size_t size;
> +	const char *path;
> +};

The virtual functions combined with the union are a funny mix of object-oriented and procedural code. The bulk_checkin_source has totally virtualized functions, but knows about all of the ancillary data each set of virtualized functions might want. ;)

I think the more pure OO version would embed the parent, and have each concrete type define its own struct type, like:

  struct bulk_checkin_source_fd {
	struct bulk_checkin_source src;
	int fd;
  };

That works great if the code which constructs it knows which concrete type it wants, and can just do:

  struct bulk_checkin_source_fd src;
  init_bulk_checkin_source_from_fd(&src, ...);

If even the construction is somewhat virtualized, then you are stuck with heap constructors like:

  struct bulk_checkin_source *bulk_checkin_source_from_fd(...);

Not too bad, but you have to remember to free now.

Alternatively, I think some of our other OO code just leaves room for a type-specific void pointer, like:

  struct bulk_checkin_source {
	...the usual stuff...

	void *magic_type_data;
  };

and then the init_bulk_checkin_source_from_fd() function allocates its own heap struct for the magic_type_data field and sticks the int in there.

That said, both of those are a lot more annoying to use in C (more boilerplate, more casting, and more opportunities to get something wrong, including leaks). So I don't mind this in-between state. It is a funny layering violating from an OO standpoint, but it's not like we expect an unbounded set of concrete types to "inherit" from the source struct.

-Peff