Re: [PATCH] commit-slab: clarify slabname##_peek()'s return value
- From
Jeff King <peff@peff.net>
- Date
- Mar 10, 2020, 18:07 UTC
- Message-ID
- <20200310180748.GC549010@coredump.intra.peff.net>
- In-Reply-To
- <20200310175446.GB549010@coredump.intra.peff.net>
On Tue, Mar 10, 2020 at 01:54:46PM -0400, Jeff King wrote:
Show 9 quoted lines
> I also wondered if we could make life easier for the caller by > collapsing these cases. I.e., always returning a zero-initialized value, > and never NULL. > [...] > But that would get a bit awkward, because peek() returns a pointer, not > a value (as it should, because the type we're storing may be a compound > type, which we generally avoid passing or returning by value). So we'd > actually need to return a pointer to a zero-initialized dummy value. Not > impossible, but getting a bit odd.
I was a little curious how it would look. Code-wise it's not too bad, but if anybody ever wrote to the dummy entry, that would cause confusing bugs. Possibly peek() could return a pointer-to-const.
The code savings in the callers are not all that much (I didn't convert all sites, but you can see that it just saves a few lines in each case).
So it's probably not worth the churn, but if anybody is really excited about it, I can polish it into a real patch.
diff --git a/blame.c b/blame.c index 29770e5c81..3ae42a1edd 100644 --- a/blame.c +++ b/blame.c @@ -15,11 +15,7 @@ static struct blame_suspects blame_suspects; struct blame_origin *get_blame_suspects(struct commit *commit) { - struct blame_origin **result; - - result = blame_suspects_peek(&blame_suspects, commit); - - return result ? *result : NULL; + return *blame_suspects_peek(&blame_suspects, commit); } static void set_blame_suspects(struct commit *commit, struct blame_origin *origin) diff --git a/commit-slab-decl.h b/commit-slab-decl.h index adc7b46c83..4b951e7b2f 100644 --- a/commit-slab-decl.h +++ b/commit-slab-decl.h @@ -13,6 +13,7 @@ struct slabname { \ unsigned stride; \ unsigned slab_count; \ elemtype **slab; \ + elemtype dummy; \ } /* diff --git a/commit-slab-impl.h b/commit-slab-impl.h index 5c0eb91a5d..505c21599a 100644 --- a/commit-slab-impl.h +++ b/commit-slab-impl.h @@ -22,6 +22,7 @@ scope void init_ ##slabname## _with_stride(struct slabname *s, \ s->slab_size = COMMIT_SLAB_SIZE / elem_size; \ s->slab_count = 0; \ s->slab = NULL; \ + memset(&s->dummy, 0, sizeof(elemtype)); \ } \ \ scope void init_ ##slabname(struct slabname *s) \ @@ -50,15 +51,15 @@ scope elemtype *slabname## _at_peek(struct slabname *s, \ if (s->slab_count <= nth_slab) { \ unsigned int i; \ if (!add_if_missing) \ - return NULL; \ + return &s->dummy; \ REALLOC_ARRAY(s->slab, nth_slab + 1); \ for (i = s->slab_count; i <= nth_slab; i++) \ s->slab[i] = NULL; \ s->slab_count = nth_slab + 1; \ } \ if (!s->slab[nth_slab]) { \ if (!add_if_missing) \ - return NULL; \ + return &s->dummy; \ s->slab[nth_slab] = xcalloc(s->slab_size, \ sizeof(**s->slab) * s->stride); \ } \ diff --git a/commit.c b/commit.c index a6cfa41a4e..49d86435a5 100644 --- a/commit.c +++ b/commit.c @@ -289,11 +289,6 @@ const void *get_cached_commit_buffer(struct repository *r, const struct commit * { struct commit_buffer *v = buffer_slab_peek( r->parsed_objects->buffer_slab, commit); - if (!v) { - if (sizep) - *sizep = 0; - return NULL; - } if (sizep) *sizep = v->size; return v->buffer;