[libvirt] [PATCH 29/29] Require space after cast

[Daniel P. Berrangé]

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 :|