On Tue, Sep 01, 2015 at 08:32:12PM -0400, Russell Bryant wrote:
> On 09/01/2015 04:35 PM, Ben Pfaff wrote:
> > Reviewing the ovn-controller code I started to notice a common pattern:
> >
> > struct smap ext_ids = SMAP_INITIALIZER(&ext_ids);
> > smap_add(&ext_ids, "ovn-patch-port", network);
> > ovsrec_port_set_external_ids(port, &ext_ids);
> > smap_destroy(&ext_ids);
> >
> > This seemed like a bit too much code for something as simple as
> > initializing an smap with a single key-value pair. This commit allows the
> > code to be reduced to just:
> >
> > const struct smap ids = SMAP_INIT1(&ids, "ovn-patch-port", network);
> > ovsrec_port_set_external_ids(port, &ids);
> >
> > This new form also eliminates multiple memory allocation and free
> > operations, but I doubt that has any real effect on performance;
> > the primary goal here is code readability.
>
> I like that the code is so much shorter. However, I'm not sure it's any
> more readable to me. The main thing that bothers me is that
> smap_destroy() is now only used *some* of the time, where it used to be
> nice and consistent.
>
> I wonder if there's something we could do with the macro name that would
> make it more clear that it doesn't need to be destroyed. SMAP_LOCAL_INIT1?
I had the same thoughts when I was writing this. My first thought was
that it should be written so that it is impossible to assign it to a
non-"const" struct hmap. However, I don't think that's actually
possible because it's always possible to convert a "const" literal to a
non-const one. That is,
int x = (const int) 123;
is valid, and so would be:
struct hmap map = (const struct hmap) { ... };
Still, the intent here is that the hmap target should be const. If that
is followed, then calling hmap_destroy() will produce a compiler error
message. If that is not followed, then hmap_destroy() becomes a no-op
(because the initializer sets hmap.buckets to &hmap.one, and in that
case hmap_destroy() does nothing) but smap_destroy() should cause a
segfault due to freeing non-allocated storage, so I think the problem
would be found on the first test.
My first name for this was SMAP_CONST_INIT1, but it was too long in my
opinion; it forces lines elsewhere to be split. Do you like
SMAP_CONST1?
> I'm just nit picking, though ... I think it's still an overall net benefit.
>
> Acked-by: Russell Bryant <rbry...@redhat.com>
Thanks. I'll apply this once the discussion seems to have concluded.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
