netdev - Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 26 Sep 2013 12:41:48 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Fabio Porcedda <fabio.porcedda@...il.com>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Dan Williams <dcbw@...hat.com>
Subject: Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

Fabio Porcedda <fabio.porcedda@...il.com> writes:

> Hi Bjørn,
> thanks for reviewing.
>
> On Wed, Sep 25, 2013 at 3:31 PM, Bjørn Mork <bjorn@...k.no> wrote:
>> Sorry, I really don't see the point of this.  Yes, the lines are longer
>> than 80 columns, but breaking them don't improve the readability at
>> all.  On the contrary, IMHO.
>>
>> So NAK from me for this part.
>
> Which changes do you think do not improve the readability?

Anything that breaks a previously unbroken argument list will reduce the
readability in my opinion.  The lines can of course not be unlimited,
but there is no need to set the limit as low as 80 columns.  Feedback
I've got from developers using e.g. 80 column braille devices is that
longer lines isn't really a problem for them either.

The point is that the properly broken lines aren't that much more
readable than a line broken by your terminal, even if you set it to 80
columns.  You do of course not get the proper indendation of the broken
lines, but you get a terminal hint telling you that it is a continuation
line.  Which is often better that having to figure it out based on the
code.

This isn't to say that I don't think writing shorter lines is a goal.
I'd really like the code to be nice and compact, avoiding this
discussion completely by just keeping every line shorter than 80.  But
I'm just not that good a programmer :-)

I'd surely appreciate any patch moving the code in that direction.

> I'm sure that at least some of them actually improve the readability.

Yes, reformatting the comments improves the readability.  I just don't
think that makes any sense on it's own, if the surrounding lines are
kept longer.

If the code can be rewritten to actually *be* shorter than 80 columns
instead of just breaking too long code lines, then that is a definite
improvement.  You didn't have many examples of this, I believe.  But
reducing the indent level, removing unnecessary function parameters, or
splitting declaration and initialization are changes which often reduce
the line length while improving the readability at the same time.
Breaking lines rarely do.

This is the only one of your code changes which I can be convinced to
agreeing may improve readability:

-     if ((on && atomic_add_return(1, &info->pmcount) == 1) || (!on && atomic_dec_and_test(&info->pmcount))) {
+     if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
+         (!on && atomic_dec_and_test(&info->pmcount))) {

But it mostly points out a piece of code which is too complex in the
first place, begging to be rewritten in a more elegant form.

Just for the record:  All this is of course only my personal opinions,
and I am fine with being overridden even if I originally wrote the ugly
code :-)

David decides.

Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html