[ovs-dev] [PATCH ovn v2 1/2] lflow.c: Avoid adding redundant resource refs for port-bindings.

[Han Zhou]

On Thu, Sep 17, 2020 at 12:49 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 9/16/20 10:05 PM, Han Zhou wrote:
> >> > Same as above. In addition, I prefer fewer functions because it is
> >> > easier (slightly) to tell from the prototypes that those are the only
> >> > operations possible for this data structure. In this case, it tells
that
> >> > this is the only interface that allocates memory for the resource
> >> > reference table. Of course with extra functions you can still tell
that
> >> > by checking the code and realize that there is only one place the
> >> > _create() functions are called, but to me that is just more noise
> >> > (slightly). I don't think every data structure needs all the
> >> > constructors and destructors in C. The code should evolve naturally
when
> >> > such functions are necessary. But if there is a coding style kind of
> >> > agreement that makes it a convention then I would just follow.
Again, I
> >> > don't have a strong opinion in this case.
> >> >
> >>
> >> I tend to agree in general but my concern in this specific case is that
> >> it's very easy to make a typo and write "free(lfrn);" instead of
> >> "free(lrln);".
> >>
> > True. But that's more of a variable naming problem. Some may argue that
> > it is easy to call the wrong function if the function names are similar
:)
>
> Well, even if the function names are similar, they would be operating on
> different data types so the compiler would complain if we'd try to call
> the wrong function.
>
> While free(void *) compiles with any pointer arg :)

I meant some like:

ref_lflow_node_destroy(rlfn);
v.s.
lflow_ref_node_destroy(lfrn);

Or even worse:

rl_node_destroy(rlfn);
v.s.
lr_node_destroy(lfrn);

Both would compile, although functions are used. On the other hand, it is
less likely to make a mistake even with just free:

free(resource_node);
v.s.
free(logical_flow_node);

So I'd say the real ones to blame in such cases are probably the naming +
careless programmer and reviewer :)

>
> In any case, I'll give this more thought coming weeks and I'll try to
> see if I can come up with a concrete patch we can discuss on.

Appreciate it!

>
> Thanks,
> Dumitru
>