* RE: CodingStyle parenthesis alignment: was: Re: align to open paren
2015-04-15 21:06 ` CodingStyle parenthesis alignment: was: Re: align to open paren Joe Perches
@ 2015-04-15 21:54 ` Hubbe, Allen
2015-04-16 1:29 ` Joe Perches
0 siblings, 1 reply; 3+ messages in thread
From: Hubbe, Allen @ 2015-04-15 21:54 UTC (permalink / raw)
To: Joe Perches; +Cc: apw@canonical.com, LKML, netdev
[-- Attachment #1: Type: text/plain, Size: 6660 bytes --]
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Wednesday, April 15, 2015 5:07 PM
> To: Hubbe, Allen
> Cc: apw@canonical.com; LKML; netdev
> Subject: CodingStyle parenthesis alignment: was: Re: align to open paren
>
> On Wed, 2015-04-15 at 20:34 +0000, Hubbe, Allen wrote:
> > Hello Andy, Joe,
>
> Hello Allen.
>
> As this is a discussion better suited for linux
> development lists I've cc'd LKML and netdev.
>
> > I would like to find the origin of the decision to align to the open
> > paren in Linux.
>
> Mostly it's a style decision shared by net/ and
> drivers/net/ and a few other directories.
>
> It's a checkpatch --strict only test so it's not on
> by default except in net/ and drivers/net/.
>
Thanks.
> > I found the commit where this was added a few years ago, d1fe9c0.
> > The email thread just says the style should be that way, but not why
> > or how the decision was made. The how and the why is what I'm after,
> > since I have a critique of the chosen style.
> >
> > I realize there is a lot of code written using this stile, and
> > changing it would be disruptive. I certainly would not ask for that.
> >
> > Indenting to the open paren might cause ambiguous indentation between
> > the parenthesized expression and the next logical thing. In the
> > following, next_thing aligns to the same column as baz, even though
> > baz is part of the condition expression, and next_thing is the
> > continued statement.
> >
> > = if (foo(bar,
> > = baz))
> > = next_thing();
> >
> > It would be necessary to reindent to maintain style, even though the
> > code of the next lines is the same. This has consequences like
> > changing the blame, for instance. In the following, 4 + 5 is the bug,
> > but whoever replaced the global with an instance variable gets the
> > blame.
>
> blame is overrated.
> git blame -w ignores most of the whitespace noise.
>
> > = global_variable = foo(bar,
> > = baz(1 + 3),
> > = baz(4 + 5) + 6);
> > with
> > = obj->var = foo(bar,
> > = baz(1 + 3),
> > = baz(4 + 5) + 6);
> >
> > I'm used to the default in many editors, which is to indent twice
> > following each open paren, as opposed to once following each open
> > brace or continued statement. It is a simpler rule for automatic
> > formatting to implement. It also avoids mixing tabs and spaces, the
> > spacing is unambiguous, and to maintain style, there is no need to
> > re-indent lines following an edit if the position of the open paren
> > changes.
> >
> > It's interesting to me that this style is only enforced by
> > checkpatch.pl --strict. It is not in Documents/CodingStyle. That
> > being the case, would it be acceptable to relax the rule in
> > checkpatch.pl to accept either style? If not, well, I'll just accept
> > the chosen style and fix my code.
>
> I personally don't care much either way.
>
> > If the following looks acceptable to you, I'll submit the patch to the
> > list.
>
> The patch most likely wouldn't be appropriate for
> net/ and drivers/net/ where the developers seem to
> strongly prefer alignment to open parenthesis.
>
I don't want to annoy the net developers. Is it that the style is the same across the kernel, just more strongly enforced in net, or do different subsystem maintainers enforce different styles? Again, I'll be happy to just accept the kernel style if that's the case.
> Also I think the open parenthesis count isn't right
> as it would ask for multiple indents for code like:
>
> if ((foo(bar)) &&
> (baz(bar))) {
As this is properly indented to the open paren, it is accepted.
>
> I think checkpatch would now want:
>
> if ((foo(bar)) &&
> (baz(bar))) {
>
CHECK: Alignment should match open parenthesis, or be twice indented for each open parenthesis
#51: FILE: foo.c:51:
+ if ((foo(bar)) &&
+ (baz(bar))) {
I count three opens and two closes in the first line, leaving one open paren, but the second line is further indented by four instead of two. That is rejected, but it accepts this:
if ((foo(bar)) &&
(baz(bar))) {
> and the --fix option would be wrong too.
I ran with the --fix option, and it changed every rejected indent to match the column of the open paren. That's probably what you want, since it's the most consistent with the previous behavior. The difference is that it does not fix lines that are no longer rejected, like if they are indented by two per paren. The test file and output are attached.
>
> cheers, Joe
>
> > Thanks,
> > Allen
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d124359..8e49125 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1834,6 +1834,15 @@ sub pos_last_openparen {
> > return length(expand_tabs(substr($line, 0, $last_openparen)))
> + 1;
> > }
> >
> > +sub count_openparen {
> > + my ($line) = @_;
> > +
> > + my $opens = $line =~ tr/\(/\(/;
> > + my $closes = $line =~ tr/\)/\)/;
> > +
> > + return $opens - $closes;
> > +}
> > +
> > sub process {
> > my $filename = shift;
> >
> > @@ -2539,11 +2548,16 @@ sub process {
> > " " x ($pos % 8);
> > my $goodspaceindent = $oldindent . " "
> x $pos;
> >
> > + my $goodtwotabindent = $oldindent .
> > + "\t\t" x
> count_openparen($rest);
> > +
> > if ($newindent ne $goodtabindent &&
> > - $newindent ne $goodspaceindent) {
> > + $newindent ne $goodspaceindent &&
> > + $newindent ne $goodtwotabindent) {
> >
> > if
> (CHK("PARENTHESIS_ALIGNMENT",
> > - "Alignment should
> match open parenthesis\n" . $hereprev) &&
> > + "Alignment should
> match open parenthesis, " .
> > + "or be twice indented
> for each open parenthesis\n" . $hereprev) &&
> > $fix && $line =~ /^\+/) {
> > $fixed[$fixlinenr] =~
> > s/^\+[
> \t]*/\+$goodtabindent/;
>
>
[-- Attachment #2: foo.c --]
[-- Type: text/plain, Size: 624 bytes --]
int main() {
if ((foo(bar)) && (baz(bar))) {
}
/* indent to open paren */
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* correct */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* indent by number of tabs */
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* correct */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
}
[-- Attachment #3: foo.c.EXPERIMENTAL-checkpatch-fixes --]
[-- Type: application/octet-stream, Size: 639 bytes --]
int main()
{
if ((foo(bar)) && (baz(bar))) {
}
/* indent to open paren */
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* correct */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* indent by number of tabs */
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* correct */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
/* incorrect */
if ((foo(bar)) &&
(baz(bar))) {
}
}
^ permalink raw reply [flat|nested] 3+ messages in thread