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

[Martin Kletzander]

On Wed, Apr 25, 2018 at 08:48:29AM +0100, Daniel P. Berrangé wrote:
>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
>

Great, I'll do that.

>> > 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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180425/1a3cfe91/attachment-0001.sig>