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.bu...@oracle.com>
> > Signed-off-by: Knut Omang <knut.om...@oracle.com>
> > Reviewed-by: Håkon Bugge <haakon.bu...@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.
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;
> > }
> > }
> > }
