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.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
cfg.mk | 6 ++++++
docs/hacking.html.in | 9 +++++++++
2 files changed, 15 insertions(+)
diff --git a/cfg.mk b/cfg.mk
index 902178dd1c3a..967f99202481 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1050,6 +1050,12 @@ sc_prohibit_backslash_alignment:
halt='Do not attempt to right-align backslashes' \
$(_sc_search_regexp)
+sc_require_space_after_cast:
+ @prohibit='(\([a-zA-Z _]+ \*+|\(((signed|unsigned|long|short|int|char|s?size_t|void) ?)+|switch \(\(vir[a-zA-Z]*Type)\)([^ );,])' \
+ in_vc_files='*\.[chx]$$' \
+ halt='Use space after cast' \
+ $(_sc_search_regexp)
+
# We don't use this feature of maint.mk.
prev_version_file = /dev/null
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index fbeea3eb751d..df3821aa3084 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -579,6 +579,15 @@
int foo(int wizz); // Good
</pre>
+ <p>
+ There must be a single whitespace immediately following a cast to any
+ type. E.g.
+ </p>
+ <pre>
+ foo = (int)bar; // Bad
+ foo = (int) bar; // Good
+</pre>
+
<h2><a id="comma">Commas</a></h2>
<p>
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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... 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. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. >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. Just let me know so that I don't send ton of patches for no reason. Have a nice day, Martin >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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/23/2018 10:16 AM, Martin Kletzander wrote: > On Mon, Apr 23, 2018 at 02:25:26PM +0100, Daniel P. Berrangé wrote: >> 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. > > Just let me know so that I don't send ton of patches for no reason. I would also vote for no spaces - I've always seen "lack of a space after a single word in parens" as the "this is a typecast" operator (it makes sense in my addled brain). -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 > Just because it's popular does not mean it's right. >2) I asked the first libvirt developer on my left when deciding, they preferred > spaces Selection bias. > >3) My own preference. > De gustibus non est disputandum. >4) The fact that this is clearly the superior way of casting =D > [citation needed] >[1] 54.85% is more than 50%, plus it is increasing as it was 52.96% during the > first draft of this clean-up. > >Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ACK if you forbid spaces after casts instead. Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2018-04-23 at 15:04 +0200, Ján Tomko 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 > > Just because it's popular does not mean it's right. I don't think there's a right or wrong in this case: it's a pure matter of preference, which makes the popularity relevant as it's as good an indicator as any as to how many people will be annoyed by the change ;) > ACK if you forbid spaces after casts instead. I'd rather have all spaces removed, even though that wouldn't be my personal preference, than keep mixing up the two approaches. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.