Re: [PATCH v2 3/5] checkpatch: Improve --fix-inplace for TABSTOP

[Knut Omang]

On Sat, 2017-12-16 at 07:13 -0800, Joe Perches wrote:
> 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@xxxxxxxxxx>
> > Signed-off-by: Knut Omang <knut.omang@xxxxxxxxxx>
> > Reviewed-by: HÃkon Bugge <haakon.bugge@xxxxxxxxxx>
> 
> 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.

I see your point here, definitely 
- more detailed statistics needed...

> Dunno.

Let me take this out of the set in the next version
as it is really a separate issue.

> Anyone else have an opinion?


Thanks,
Knut

>
> > ---
> >  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;
> >  				}
> >  			}
> >  		}