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

> >> 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.
>
> Ah, so it is a "please trust me gcc, I know what I am doing" thing and
> not a "this is required in C" thing. Makes sense, thanks for clarifying.
>
> Sorry for the noise.

> >> 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.
>
> When functions are built on top of other functions, I think it is good
> practice to point readers to those underlying functions. In this case
> the new function is a wrapper around oidset_insert() which does all the
> real work. Plus the helper function already has some documentation about
> copying behavior that we already thought was important enough to call
> out explicitly.
>
> So, tying this definition to that (foundational) helper function sounds
> like a good idea to me in terms of readability. IOW we can inform
> readers "hey, we're just a wrapper around this other important function
> --- go there if you're curious about internals" and emphasizing that
> sort of relationship which may not be immediately obvious to those not
> familiar with this area would be nice.
>
> Alternatively, we could repeat the same comment WRT copying here but
> that seems redundant and prone to maintenance burdens down the road (if
> we ever change this behavior we have to change the comment in multiple
> functions, possibly).
>
> > 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.
>
> OK.

> >> > +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.
>
> Thank you for the additional context I was not aware of.
>
> > 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.
>
> I agree.
>
> However, I would like to point out that the function being added in this
> patch is a bit special: it is inserting from one "oidset" into another
> "oidset". IOW the both the dest and src types are the same.
>
> For the cases where the types are different, I totally agree that using
> the suffixes (to encode the type information of the src into the
> function name itself) is a good idea.
>
> So I think it's still fine to use "oidset_insert_all" because the only
> type in the parameter list is an oidset.