Message-ID: <1513437230.4647.25.camel@perches.com>
Subject: Re: [PATCH v2 3/5] checkpatch: Improve --fix-inplace for TABSTOP
From: Joe Perches <joe@perches.com>
To: Knut Omang <knut.omang@oracle.com>, linux-kernel@vger.kernel.org
Cc: Andy Whitcroft <apw@canonical.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dan Carpenter <error27@gmail.com>
Date: Sat, 16 Dec 2017 07:13:50 -0800
In-Reply-To: <054ce1000f13fb321b84ac474957a759633e5a9d.1513430008.git-series.knut.omang@oracle.com>
References: <cover.630ac8faeeda67bf7a778c158174422042942d08.1513430008.git-series.knut.omang@oracle.com>
         <054ce1000f13fb321b84ac474957a759633e5a9d.1513430008.git-series.knut.omang@oracle.com>
Content-Type: text/plain; charset="ISO-8859-1"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2286
Lines: 59

On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> If the --fix-inplace option for TABSTOP encounters a sitation with several
> spaces (but less than 8) at the end of an indentation, it will assume that there
> are extra indentation and align back to the nearest tabstop instead of the next.
> 
> This might go undetected in a "full" checkpatch --fix-inplace run, as this
> potential new error will be handled by SUSPECT_CODE_INDENT further down in the
> script.  The code for TABSTOP have limited "knowledge" of the previous line.
> Adding complexity when it is taken care of later anyway is maybe unnecessary/undesired.
> As a simple heuristics just use a "natural" rounding algorithm and round
> up for 4 spaces or more.
> 
> There's an example in line 108 in net/rds/ib_recv.c with indentation TAB + 7
> spaces where the correct would have been an additional TAB.  With only TABSTOP
> enabled, checkpatch will fix this to a single TAB, which is visually worse IMO.
> 
> Reported-by: H?kon Bugge <haakon.bugge@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> Reviewed-by: H?kon Bugge <haakon.bugge@oracle.com>

Well written commit log description, thanks but
I'm not sure that's actually better overall.

It's not possible to know if the appropriate thing
to do is to add a tab or remove spaces.

This may get it wrong about as often as the current
code gets it right.

Last I checked 6 months or so ago, there were ~8000
TABSTOP warnings in the kernel tree.

Most of those still seem valid and moving additional
places to the right is sometimes incorrect too.

It seems most common when there are 3 or 4 spaces
used as indent level indentation instead of tabs.

Dunno.

Anyone else have an opinion?

> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 040aa79..febe010 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2989,7 +2989,7 @@ sub process {
>  				if (WARN("TABSTOP",
>  					 "Statements should start on a tabstop\n" . $herecurr) &&
>  				    $fix) {
> -					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
> +					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x (($indent+4)/8)@e;
>  				}
>  			}
>  		}