On Mon, Apr 23, 2018 at 04:16:11PM +0200, Martin Kletzander wrote:
> On Mon, Apr 23, 2018 at 02:25:26PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Apr 23, 2018 at 02:44:57PM +0200, Martin Kletzander wrote:
> > > Let's make a rule out of it and document it. This is based on few sources:
> > >
> > > 1) Most of the code [1] used spaces after casts, so the patch to change it this
> > > way rather than the other way around is smaller
> > >
> > > 2) I asked the first libvirt developer on my left when deciding, they preferred
> > > spaces
> > >
> > > 3) My own preference.
> > >
> > > 4) The fact that this is clearly the superior way of casting =D
> > >
> > > [1] 54.85% is more than 50%, plus it is increasing as it was 52.96% during the
> > > first draft of this clean-up.
> >
> > I'm surprised that is the case, but if you'll show the command you used to
> > extract that stat I could be convinced...
> >
>
> with_spaces=$(git ls-files '*.[chx]' | xargs grep -E '(\([a-zA-Z _]+ \*+|\(((signed|unsigned|long|short|int|char|s?size_t|void) ?)+|switch \(\(vir[a-zA-Z]*Type)\) ([^ );,])' | wc -l)
> without_spaces=$(git ls-files '*.[chx]' | xargs grep -E '(\([a-zA-Z _]+ \*+|\(((signed|unsigned|long|short|int|char|s?size_t|void) ?)+|switch \(\(vir[a-zA-Z]*Type)\)([^ );,])' | wc -l)
> total=$(git ls-files '*.[chx]' | xargs grep -E '(\([a-zA-Z _]+ \*+|\(((signed|unsigned|long|short|int|char|s?size_t|void) ?)+|switch \(\(vir[a-zA-Z]*Type)\) ?([^ );,])' | wc -l)
> printf "Casts with spaces: %.2f%%\nCasts without spaces: %.2f%%\n" $((with_spaces * 100.0 / total)) $((without_spaces * 100.0 / total))
>
> It's kinda hairy, but it works. Except the xdrproc_t cast that Peter pointed
> out. With that fixed it is actually 52.94% instead of 54.85%. That also shows
> that the first time I did it, I certainly included the *_t in the regex.
Ah, so the reason why the with spaces count is much higher than I expected
is almost entirely down to one file - the remote driver.
Without spaces
$ git ls-files '*.[chx]' | xargs grep -E '(\([a-zA-Z _]+ \*+|\(((signed|unsigned|long|short|int|char|s?size_t|void) ?)+|switch+\(\(vir[a-zA-Z]*Type)\)([^ );,])' | awk '{print $1}' | uniq -c | sort -n | tail
18 tools/virsh-domain.c:
24 src/esx/esx_vi_types.c:
30 src/hyperv/hyperv_driver.c:
32 src/util/viratomic.h:
33 src/util/virdbus.c:
34 src/conf/domain_conf.c:
43 src/xenapi/xenapi_driver.c:
52 src/node_device/node_device_hal.c:
54 src/vbox/vbox_common.c:
56 src/remote/remote_driver.c:
With spaces
$ git ls-files '*.[chx]' | xargs grep -E '(\([a-zA-Z _]+ \*+|\(((signed|unsigned|long|short|int|char|s?size_t|void) ?)+|switch+\(\(vir[a-zA-Z]*Type)\) ([^ );,])' | awk '{print $1}' | uniq -c | sort -n | tail
14 src/security/security_dac.c:
15 src/util/virxml.c:
17 tools/virsh-pool.c:
20 src/admin/admin_remote.c:
21 tests/qemumonitorjsontest.c:
21 tests/virstoragetest.c:
24 src/qemu/qemu_driver.c:
25 src/remote/remote_daemon_dispatch.c:
44 tests/testutilshostcpus.h:
238 src/remote/remote_driver.c:
Amuzingly the remote driver has both styles on the very same line in many
cases !
Anyway, this just makes me prefer the without-spaces style more because
remote driver is an outlier
> > Personally I'm not a fan of adding the extra space - the cast is associated
> > with the variable, so I don't think it needs it.
> >
>
> I know we've discussed it earlier, and I don't want to repeat myself, but for
> the sake of keeping it in the conversation; there are spaces around everything
> except function calls, but in the end it's just a clearer separation. I used to
> prefer non-spaced casts too, but then I misread some and realized it's similar
> to arithmetics for example.
>
> And before anyone starts shouting at me that it is very subjective and it all
> depeds on x, y, and z, including your terminal font, I agree, it for sure is.
> And I also agree that we should not be spending almost any of out time with
> something that's very close to bikeshedding =) I just want this to be unified
> across the codebase, and if the consensus is that there should be no spaces,
> then I'll resend the series with the spaces removed.
Yes, I totally agree that - it is subjective and we should unify it.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
