On Tue, Jul 07, 2009 at 01:26:11PM -0400, Gregory Haskins wrote:
> These patches pass checkpatch.pl and I happen to like the extra
> whitespace for readability. I agree that a random isolated whitespace
> hunk, or double whitespace in a row are probably inadvertent and should
> be pointed out. But these little one liners in the middle of code I
> generally do on purpose (for instance, [A]).
Where it's on purpose, it's on purpose. I am just trying to convey a
rather obvious point that each line of code should have a purpose in
life, and that includes empty lines :)
> I suppose its personal preference either way, so I guess unless Avi
> objects lets just each have our own style in that regard.
I think Avi already said we don't need to standardize everything.
I hope at least some of the comments were helpful.
> >> @@ -552,6 +554,11 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
> >> * Initialize PIO device
> >> */
> >> kvm_iodevice_init(&s->dev, &picdev_ops);
> >> - kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
> >> + ret = kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
> >> + if (ret < 0) {
> >>
> >
> > I thought the function returns 0 on success?
> > If so can we just if (ret) all over?
> >
> >
>
> I guess, but what does that churn buy us?
It's not that important really. I think we need to document the return
value, and check it according to how it is documented. The reason I
commented, I see < 0 and ask "what if it is > 0"? I look it up and it
turns out it's never > 0. So why check < 0?
> >> + ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev);
> >> + if (ret < 0)
> >> + kfree(dev);
> >> +
> >>
> >
> > kill empty line
> >
>
> Why do you object here especially when there is precedence
How do you mean, precedence?
> with
> something like the space before the return with [B]? I think big
> mono-blocks of code are ugly and harder to read, personally.
I don't intend to keep arguing and I agree it's a question of style.
But since you ask why I'll try to answer. I think an
empty line should help delimit a block of code with some common meaning,
like a paragraph. But if overused it loses meaning and stops being
helpful.
E.g., it does not make sense to put it between every 2 lines of code in a
function. It also does not make sense to put it after } which is
already delimiting a block in a visible way; it does not make sense to
put it around a multiline comment which is delimited by /**/. It does
not make sense to put it around an idented block which is already
delimited by indentation.
> >> + if (bus->devs[i] == dev) {
> >> + bus->devs[i] = bus->devs[--bus->dev_count];
> >> + break;
> >> + }
> >> + }
> >>
> >
> > no {} around single statement
This is actually part of style IMO, it is just hard for a perl script to
catch :).
> >
> >
> >> }
> >>
> >> static struct notifier_block kvm_cpu_notifier = {
> >>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
