Re: [PATCH v4 1/8] target/mips: Clean up helper.c

[Markus Armbruster]

Aleksandar Markovic <address@hidden> writes:

> --00000000000007f6800594da656e
> Content-Type: text/plain; charset="UTF-8"
>
> On Monday, October 14, 2019, Markus Armbruster <address@hidden> wrote:
>
>> Aleksandar Markovic <address@hidden> writes:
>>
>> > From: Aleksandar Markovic <address@hidden>
>> >
>> > Mostly fix errors and warnings reported by 'checkpatch.pl -f'.
>> >
>> > Signed-off-by: Aleksandar Markovic <address@hidden>
>> > ---
>> >  target/mips/helper.c | 128 ++++++++++++++++++++++++++++++
>> +--------------------
>> >  1 file changed, 78 insertions(+), 50 deletions(-)
>> >
>> > diff --git a/target/mips/helper.c b/target/mips/helper.c
>> > index a2b6459..2411a2c 100644
>> > --- a/target/mips/helper.c
>> > +++ b/target/mips/helper.c
[...]
>> > @@ -130,8 +133,11 @@ static int is_seg_am_mapped(unsigned int am, bool eu, 
>> > int mmu_idx)
>> >      int32_t adetlb_mask;
>> >
>> >      switch (mmu_idx) {
>> > -    case 3 /* ERL */:
>> > -        /* If EU is set, always unmapped */
>> > +    case 3:
>> > +        /*
>> > +         * ERL
>> > +         * If EU is set, always unmapped
>> > +         */
>> >          if (eu) {
>> >              return 0;
>> >          }
>>
>> This changes from the usual way we format switch case comments to an
>> unusual way.
>>
>> If you want to pursue this change, please put it in a separate patch,
>> so this one is really about fixing "errors and warnings reported by
>> 'checkpatch.pl -f'", as your commit message promises.
>>
>>
>
> Hi, Markus. Thank you for your response.
>
> There must be some misunderstanding here:
>
> The line:
>
>    case 3 /* ERL */:
>
> generates a checkpatch warning. I don't know why I would put it in a
> separate patch, if this patch is about fixing checkpatch warnings. Please
> explain.

You're right; I misread the line you patch as

     case 3: /* ERL */

> Secondly, I don't see that this is a usual way we format switch statement.
> I found just several cases in the whole QEMU code base (and you claimed in
> previous comments that there are thousands).
>
> I am just guessing that you somehow mixed this line with the line:
>
>    case 3: /* ERL */
>
> that would have not generated checkpatch warning.

You guessed correctly.  Telling me right away that my remark doesn't
make sense to you would've helped :)

The pattern

    case VALUE: /* comment on VALUE */

is common: >8000 instances.

The pattern

    case VALUE /* comment on VALUE */:

is uncommon: <20 instances.  I agree with cleaning it up.

However, I find the common pattern applied here

    case 3: /* ERL */
        /* If EU is set, always unmapped */
        if (eu) {
            return 0;
        }

more readable than the unusual (to my eyes)

    case 3:
        /*
         * ERL
         * If EU is set, always unmapped
         */
        if (eu) {
            return 0;
        }

The first line of the comment applies to the value preceding it, the
second to the code following it.  Making these connections doesn't
exactly take genius, but neither is it effortless.

Nice and consistent coding style is all about reducing the effort of
reading code.

For what it's worth, the pattern

    case VALUE: /* comment on VALUE */
        /* comment on CODE */
        CODE

occurs almost 300 times.

> I don't see any reason to change this patch. Please let me know it you
> still think I should do something else. And you are welcome to analyse any
> patches of mine.

Please consider keeping two separate comments, i.e. just move the colon
to its usual place.

Thanks!