On Mon, 2014-04-07 at 14:10 -0400, Dan Winship wrote: > On 04/04/2014 05:46 PM, Thomas Haller wrote: > > On Fri, 2014-04-04 at 16:24 -0500, Dan Williams wrote: > >> On Fri, 2014-04-04 at 23:02 +0200, Thomas Haller wrote: > >>> On Fri, 2014-04-04 at 15:13 -0500, Dan Williams wrote: > >>>> Is the only reason for the #define of the common fields so that you > >>>> don't have to do another level of indirection? It looks somewhat ugly > >>>> and my personal preference would be to just declare the base struct in > >>>> the functions you want to use it in and up-cast if you need the v4 or v6 > >>>> version... kinda like we do with objects. So I certainly agree with > >>>> the principle, but lets see what other people say about the > >>>> implementation... > > > I see, but the disadvantage is, that I would have to fixup *many* > > occurrences in existing code. Also, it is more typing. > > But many of those existing occurrences involve separate IPv4 and IPv6 > functions / codepaths where the actual behavior is exactly the same and > ought to be merged into a single NMPlatformIPAddress codepath anyway... > > -- Dan
I would say that many occurrences are no candidates for refactoring: $ git grep '\(->\|\.\)\<lifetime\>' (which does not mean, that it isn't useful in several cases!) I think, there are several reasons for the *_COMMON define: * it would need more typing for no benefit: - addr.lifetime = lifetime; + addr.common.lifetime = lifetime; or + ((NMPlatformIPAddress *) &addr)->lifetime = lifetime; * I have another version of the patch, that combines NMPlatformLink, NMPlatform*Address and NMPlatform*Route with: +#define __NMPlatformObject_COMMON \ + int ifindex; \ + ; that would mean: - addr.ifindex = ifindex; - addr.lifetime = lifetime; + addr.common_addr.common.ifindex = ifindex; + addr.common_addr.lifetime = lifetime; or + ((NMPlatformObject *) &addr)->ifindex = ifindex; + ((NMPlatformIPAddress *) &addr)->lifetime = lifetime; * I don't want to touch existing/working code now (even if it's trivially fixing compile errors). Thomas PS: yes, I have an actual usecase for this patch
signature.asc
Description: This is a digitally signed message part
_______________________________________________ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
