Re: [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 14, 2024, 14:33 UTC
Message-ID: <CAP8UFD1aJD5i68ekHuq0UG14X19y=Eo6qKPianF8MKNf6iZ_WQ@mail.gmail.com>
In-Reply-To: <owlyle7o9iyf.fsf@fine.c.googlers.com>

On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote:

Show 15 quoted lines

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > In a following commit, we will need to add all the oids from a set into
> > another set. In "list-objects-filter.c", there is already a static
> > function called add_all() to do that.
>
> Nice find.
>
> > Let's rename this function oidset_insert_from_set() and move it into
> > oidset.{c,h} to make it generally available.
>
> At some point (I don't ask for it in this series) we should add unit
> tests for this newly-exposed function. Presumably the stuff around
> object/oid handling is stable enough to receive unit tests.

Yeah, ideally there should be unit tests for oidset and all its features, but it seems to me that there aren't any. Also oidset is based on khash.h which was originally imported from https://github.com/attractivechaos/klib/ without tests. So I think it's a different topic to add tests from scratch to oidset, khash.h or both.

Actually after taking another look, it looks like khash.h or some of its features are tested through "helper/test-oidmap.c" and "t0016-oidmap.sh". I still think it's another topic to test oidset.

Show 9 quoted lines

> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
> > +{
> > +     struct oidset_iter iter;
> > +     struct object_id *src_oid;
> > +
> > +     oidset_iter_init(src, &iter);
> > +     while ((src_oid = oidset_iter_next(&iter)))
>
> Are the extra parentheses necessary?

Yes. Without them gcc errors out with:

oidset.c: In function ‘oidset_insert_from_set’:
oidset.c:32:16: error: suggest parentheses around assignment used as
truth value [-Werror=parentheses]
  32 |         while (src_oid = oidset_iter_next(&iter))
     |                ^~~~~~~

Having extra parentheses is a way to tell the compiler that we do want to use '=' and not '=='. This helps avoid the very common mistake of using '=' where '==' was intended.

Show 7 quoted lines

> > +/**
> > + * Insert all the oids that are in set 'src' into set 'dest'; a copy
> > + * is made of each oid inserted into set 'dest'.
> > + */
>
> Just above in oid_insert() there is already a comment about needing to
> copy each oid.

(It's "oidset_insert()" not "oid_insert()".)

Show 12 quoted lines

>     /**
>      * Insert the oid into the set; a copy is made, so "oid" does not need
>      * to persist after this function is called.
>      *
>      * Returns 1 if the oid was already in the set, 0 otherwise. This can be used
>      * to perform an efficient check-and-add.
>      */
>
> so perhaps the following wording is simpler?
>
>     Like oid_insert(), but insert all oids found in 'src'. Calls
>     oid_insert() internally.

(What you suggest would need s/oid_insert/oidset_insert/)

Yeah, it's a bit simpler and shorter, but on the other hand a reader might have to read both this and the oidset_insert() doc, so in the end I am not sure it's a big win for readability. And if they don't read the oidset_insert() doc, they might miss the fact that we are copying the oids we insert, which might result in a bug.

Also your wording ties the implementation with oidset_insert(), which we might not want if we could find something more performant. See Junio's comment on this patch saying his initial reaction was that copying underlying bits may even be more efficient.

So I prefer not to change this.

Show 6 quoted lines

> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
>
> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
> to reuse any descriptors in comments to guide the names. Plus this
> function used to be called "add_all()" so keeping the "all" naming style
> feels right.

We already have other related types like 'struct oid-array' and 'struct oidmap' to store oids, as well as code that inserts many oids into an oidset from a 'struct ref *' linked list or array in a tight loop. So if we want to add functions inserting all the oids from instances of such types, how should we call them?

I would say we should use suffixes like: "_from_set", "_from_map", "from_array", "_from_ref_list", "_from_ref_array", etc.

If we start using just "_all" for oidset, then what should we use for the other types? I don't see a good answer to that, so I prefer to stick with "_from_set" for oidset.

Thanks.