Re: [PATCH v2 5/6] object_array: add and use `object_array_pop()`

["Martin Ågren" <martin.agren@gmail.com>]

On 23 September 2017 at 06:27, Jeff King <peff@peff.net> wrote:
> On Sat, Sep 23, 2017 at 01:34:53AM +0200, Martin Ågren wrote:
>
>> Introduce and use `object_array_pop()` instead. Release memory in the
>> new function. Document that popping an object leaves the associated
>> elements in limbo.
>
> The interface looks appropriate for all of the current cases. Though I
> do suspect there's a bit of catch-22 here. If a caller _did_ care about
> the "name" and "path" fields, then this pop function would be
> inappropriate, because it returns only the object field.
>
> So in the most general form, you'd want:
>
>   while (foo.nr) {
>           struct object_array_entry *e = object_array_pop(&foo);
>
>           ... do stuff with e->name, etc ...
>
>           object_array_release_entry(e);
>   }
>
> But that is certainly more cumbersome for these callers. I think we can
> punt on that until it becomes necessary (which likely is never).

I considered something like this. I felt that making the function
`object_array_release_entr()` available to the general public would also
carry some risk. Right now, it's only object.c that needs to get things
right (never release twice, never release without removing, never be
clever, ..)

>> The name of `object_array_pop()` does not quite align with
>> `add_object_array()`. That is unfortunate. On the other hand, it matches
>> `object_array_clear()`. Arguably it's `add_...` that is the odd one out,
>> since it reads like it's used to "add" an "object array". For that
>> reason, side with `object_array_clear()`.
>
> Yes, we're dreadfully inconsistent here. I tend to prefer noun_verb()
> when "noun" is a struct we're operating on. But we have quite a bit of
> verb_noun(). I find that noun_verb() is a bit more discoverable (e.g.,
> tab completion does something sensible), but I'm not sure if it's worth
> trying to do a mass-conversion.
>
> Perhaps it's something that should be mentioned in CodingGuidelines, if
> it isn't already.

When writing the above paragaph (so yes, after I had already chosen the
name), I tried to find something, but couldn't. Admittedly, I just had a
cursory look.

> The patch itself looks good, with one tiny nit:
>
>> diff --git a/object.h b/object.h
>> index 0a419ba8d..b7629fe92 100644
>> --- a/object.h
>> +++ b/object.h
>> @@ -115,6 +115,13 @@ int object_list_contains(struct object_list *list, struct object *obj);
>>  /* Object array handling .. */
>>  void add_object_array(struct object *obj, const char *name, struct object_array *array);
>>  void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);
>> +/*
>> + * Returns NULL if the array is empty. Otherwise, returns the last object
>> + * after removing its entry from the array. Other resources associated
>> + * with that object are left in an unspecified state and should not be
>> + * examined.
>> + */
>> +struct object *object_array_pop(struct object_array *array);
>
> I'm very happy to see a comment over the declaration here. But I think
> it's a bit more readable if we put a blank line between the prior
> function and the start of that comment.

Yes, that looks strange. :( I could re-roll and/or ask Junio to fiddle
with it. On closer look, this file is pretty close to documenting all
functions and there are some other comment-formatting issues. So here's
a promise that I'll get back to clean this up more generally in the not
too distant future. Would that be an acceptable punt? :-?

Martin