Re: [PATCH 03/12] shallow.c: use commit-slab for commit depth instead of commit->util

From: Jeff King <peff@peff.net>
Date: May 12, 2018, 09:18 UTC
Message-ID: <20180512091809.GC28279@sigill.intra.peff.net>
In-Reply-To: <20180512090748.GB28279@sigill.intra.peff.net>

On Sat, May 12, 2018 at 05:07:48AM -0400, Jeff King wrote:

Show 11 quoted lines

> So no, it wouldn't work to directly store depths with the code as
> written.  I'm not sure if the depth can ever be 0. If not, then it would
> be a suitable sentinel as:
> 
>   int *slot = commit_depth_at(&depths, p->item);
>   if (!*slot || cur_depth < *slot)
> 	*slot = cur_depth;
> 
> But somebody would have to dig into the possible values of cur_depth
> there (which would make sense to do as a separate patch anyway, since
> the point of this is to be a direct conversion).

By the way, one other approach if xcalloc() doesn't produce a good sentinel is to use a data type that does. ;) E.g., something like this should work:

  struct depth {
	unsigned valid:1;
	int value;
  };
  define_commit_slab(commit_depth, struct depth);

...

  struct depth *slot = commit_depth_at(&depths, p->item);
  if (!slot->valid || cur_depth < slot->value) {
	slot->value = cur_depth;
	slot->valid = 1;
  }

That wastes an extra 4 bytes per slot over storing an int directly, but it's the same as storing an 8-byte pointer, plus you avoid the storage and runtime overhead of malloc.

I actually wonder if we could wrap commit_slab with a variant that stores the sentinel data itself, to make this pattern easier. I.e., something like:

  #define define_commit_slab_sentinel(name, type) \
	struct name##_value { \
		unsigned valid:1; \
		type value; \
	}; \
	define_commit_slab(name, struct name##_value)

and some matching "peek" and "at" functions to manipulate value directly.

I think the end result would be nicer, but it's turning into a little bit of a rabbit hole. So I don't mind going with your direct conversion here for now.

-Peff