On Sat, May 12, 2018 at 11:22 AM, Jeff King <peff@xxxxxxxx> wrote: > On Sat, May 12, 2018 at 10:00:17AM +0200, Nguyễn Thái Ngọc Duy wrote: > >> +define_commit_slab(blame_suspects, struct blame_origin *); >> +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; >> +} > > Hmm. You need this helper because you want to be able to peek and get a > NULL. But that's already what _at() would return, with the only > difference that we may extend the slab just to return NULL. > > I wonder how much it matters in practice. We'd generally be extending > the slab to hit every commit anyway in this case, I would think. I don't know much about blame so I stay very conservative ;-) If it's safe to just do _at() here, I'll update this patch. > I suppose it doesn't actually simplify the code that much to do it that > way, though. We could get rid of this helper, but the caller would still > look like: > > for (p = *blame_suspects_at(o->commit); p; p = p->next) > > which is actually slightly uglier than get_blame_suspects(), because we > have to do the pointer-dereference ourselves. And the caller would need to include commit-slab.h too. I added get_blame_suspects() because I wanted to avoid that. -- Duy