Messages in this thread | | | Subject | Re: [PATCH v2 3/5] checkpatch: Improve --fix-inplace for TABSTOP | From | Knut Omang <> | Date | Sat, 16 Dec 2017 16:55:56 +0100 |
| |
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@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.
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; > > } > > } > > }
| |