Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Zhangjin Wu <falcon@tinylab.org>
To:     w@1wt.eu
Cc:     arnd@arndb.de, david.laight@aculab.com, falcon@tinylab.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-riscv@lists.infradead.org, thomas@t-8ch.de
Subject: Re: [PATCH v5 06/14] tools/nolibc: arch-*.h: clean up multiple whitespaces
Date:   Mon,  3 Jul 2023 22:02:00 +0800
Message-Id: <20230703140200.499769-1-falcon@tinylab.org>
In-Reply-To: <20230702184453.GF16233@1wt.eu>
References: <20230702184453.GF16233@1wt.eu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrgz:qybglogicsvrgz5a-1
Precedence: bulk

Hi, Willy

> Hi Zhangjin,
> 
> On Wed, Jun 28, 2023 at 09:19:33PM +0800, Zhangjin Wu wrote:
> > To align with Linux code style and let scripts/checkpatch.pl happy, the
> > multiple whitespaces in arch-<ARCH>.h files are cleaned up.
> > 
> > Most of them are modified by these commands automatically:
> > 
> >     $ sed -i -e '/#define my_syscall/,/})/{s/        /\t/g}' tools/include/nolibc/arch-*.h
> >     $ sed -i -e '/#define my_syscall/,/})/{s/ *\\$/\t\\/g}' tools/include/nolibc/arch-*.h
> > 
> > And checked with:
> > 
> >     $ grep '  *\\$' tools/include/nolibc/arch-*.h
> 
> I'm surprised by this one, I never saw checkpatch complain here. For me,
> putting a tab after a non-tab is an error. It makes the code harder to
> edit and re-align, and diffs are harder to read on lines whose lengths
> varies by +/-1 around a multiple of 8 as it makes the post-tab stuff
> zigzag. You made me recheck the coding style file, and there's nothing
> about alignment there, only about indent (and indent uses tabs here).
> There are also other parts which use spaces for alignment (albeit not
> that many), so unless there is a solid reason for changing that, I'd
> rather not do it, as for me it's the exact opposite of a cleanup as it
> will cause me quite some discomfort.
>

Willy, it is not about alignment, just rechecked it, it is code indent
related:

    #32: FILE: tools/include/nolibc/arch-mips.h:160:
    +^I                                                                      \$
    
    ERROR: code indent should use tabs where possible
    #44: FILE: tools/include/nolibc/arch-mips.h:172:
    +^I          "t0", "t1", "t2", "t3", "t4", "t5", "t6", "t7", "t8", "t9"  \$

The first one is here:

	register long _arg6 = (long)(arg6);                                   \
	<--     whitespaces                                                -->\
	__asm__  volatile (                                                   \

And the second one:

		: "memory", "cc", "at", "v1", "hi", "lo",                     \
	<-spaces->"t0", "t1", "t2", "t3", "t4", "t5", "t6", "t7", "t8", "t9"  \

These two lines indent with "one tab + more than 8 whitespaces", the
other lines also have whitespaces code indent, but not more than 8, so,
not reported.

I have tried to replace whitespaces with tabs in the first one, but the first
one can not align with the other lines, so, the current modify method is
applied.

To only touch minimal lines, this may work (reserve the post-whitespaces):

    $ sed -i -e '/^\t*        /{s/        /\t/g}' tools/include/nolibc/arch-*.h

It will only fix up the lines the reported. The cleanup of the post-whitespaces
is not necessary and it does touch too many lines.

Sorry to disturb you with such cleanups, since I have seen the similar
reports when we added the arch-arm.h (for my_syscall6), because the code
style aligns with the others, so, I did touch it, but again encounter
the same issues with arch-mips.h and to avoid the future reports, I
checked the whole arch-xxx.h, and found more such reports, so, we
prepared such a patch.

To be honest, I do prefer post-tabs to whitespaces (less key press, less code
size ;-)), but as you pointed out, post-tabs have more side-effects, we
shouldn't touch them, Thanks.

> > Besides, more multiple whitespaces are cleaned up:
> > 
> > - convert "__asm__  volatile" to "__asm__ volatile"
> 
> I totally agree on this one, it's very likely the result of a mechanical
> change.

Ok, will split it to a standalone patch, one error report one patch.

> 
> > - "foo _num  bar" should be "foo _num bar"
> 
> In theory yes, except that for those where it appears it was only to
> keep all declarations aligned given that this _num was shorter by one
> char than all other local names. Especially when it comes to enumerating
> register names, you definitely want to keep them aligned. It's sufficiently
> difficult to avoid mistakes there, any help for visual check counts.
>

Agree, let's keep it as before.

Thanks,
Zhangjin

> Willy