Le 28/02/2022 à 11:56, Petr Mladek a écrit :
> On Fri 2022-02-25 16:49:31, Christophe Leroy wrote:
>>
>>
>> Le 25/02/2022 à 10:34, Petr Mladek a écrit :
>>>
>>> Please do not do these small coding style changes. It complicates the
>>> review and increases the risk of regressions. Different people
>>> have different preferences. Just imagine that every half a year
>>> someone update style of a code by his personal preferences. The
>>> real changes will then get lost in a lot of noise.
>>
>> I disagree here. We are not talking about people's preference here but
>> compliance with documented Linux kernel Codying Style and handling of
>> official checkpatch.pl script reports.
>
> Really?
>
> 1. I restored
>
> + if (mod->klp_info->secstrings == NULL) {
>
> and checkpatch.pl is happy.
On mainline's kernel/module.c checkpatch.pl tells me:
CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
#2092: FILE: kernel/module.c:2092:
+ if (mod->klp_info->secstrings == NULL) {
>
>
> 2. I do not see anythinkg about if (xxx == NULL) checks in
> Documentation/process/coding-style.rst
>
> 3. $> git grep "if (.* == NULL" | wc -l
> 15041
Commit b75ac618df75 ("checkpatch: add --strict "pointer comparison to
NULL" test")
>
> 4. The result of
> - mod->klp_info->sechdrs[symndx].sh_addr = \
> - (unsigned long) mod->core_kallsyms.symtab;
> + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab;
>
> is 90 characeters long and Documentation/process/coding-style.rst says:
Probably a misinterpretation of:
WARNING: Avoid unnecessary line continuations
#2107: FILE: kernel/module.c:2107:
+ mod->klp_info->sechdrs[symndx].sh_addr = \
>
> 2) Breaking long lines and strings
> ----------------------------------
>
> Coding style is all about readability and maintainability using commonly
> available tools.
>
> The preferred limit on the length of a single line is 80 columns.
>
> Statements longer than 80 columns should be broken into sensible chunks,
> unless exceeding 80 columns significantly increases readability and does
> not hide information.
>
> checkpatch.pl accepts lines up to 100 columns but 80 are still
> preferred.
>
>
>> You are right that randomly updating the style every half a year would
>> be a nightmare and would kill blamability of changes.
>>
>> However when moving big peaces of code like this, blamability is broken
>> anyway and this is a very good opportunity to increase compliance of
>> kernel code to its own codying style. But doing it in several steps
>> increases code churn and has no real added value.
>
> From Documentation/process/submitting-patches.rst:
>
> One significant exception is when moving code from one file to
> another -- in this case you should not modify the moved code at all in
> the same patch which moves it. This clearly delineates the act of
> moving the code and your changes. This greatly aids review of the
> actual differences and allows tools to better track the history of
> the code itself.
>
>
>>>
>>> Coding style changes might be acceptable only when the code is
>>> reworked or when it significantly improves readability.
>>
>> When code is moved around it is also a good opportunity.
>
> No!
By the way some maintainers require checkpatch' clean patches even when
this is only code move. I remember being requested to do that in the
past, so now I almost always do it with my own patches.
>
> I would not have complained if it did not complicate my review.
> But it did!
>
Reviewing partial code move is not easy anyway, git is not very
userfriendly with that.
Christophe
