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-*.hAnd checked with:
$ grep ' *\\$' tools/include/nolibc/arch-*.hI'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