Re: [PATCH v2 01/20] target/mips: Clean up helper.c

[Markus Armbruster]

Aleksandar, your message is hard to read, because you sent Content-Type:
multipart/alternative, and both the test/html and the test/plain
alternative ride roughshot over the quoted text's line structure.
Quoted patches become unreadable garbage.  Please check your e-mail
setup.

Aleksandar Markovic <address@hidden> writes:

> 25.09.2019. 17.53, "Philippe Mathieu-Daudé" <address@hidden> је
> написао/ла:
>>
>> On 9/25/19 2:45 PM, Aleksandar Markovic wrote:
>> > 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 | 132
> +++++++++++++++++++++++++++++++--------------------
>> >  1 file changed, 80 insertions(+), 52 deletions(-)
>> >
>> > diff --git a/target/mips/helper.c b/target/mips/helper.c
>> > index a2b6459..3dd1aae 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
>> > +         */
>>
>> My IDE show the current form nicer when the switch is folded.
>>
>> Are these comment really bothering checkpatch?
>>
>
> While being sintaxically correct, interleaving comments and code in a
> single code line is considered a bad practice by many.

I take that as a "no".

The preferences of "many" (whoever they may be) are a lot less relevant
than the specific project's prevailing style.  "git-grep ' case .*/\*'"
shows thousands of hits.

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.

>> >          if (eu) {
>> >              return 0;
>> >          }
[...]
>> Except 2 comments, OK for the rest.
>>
>> Another patch that makes rebasing very painful :(
>>
>
> It would be fantastic if you apply the same reasoning to your patches, that
> spread all over the code base, and happen so frequently, and certainly
> create enormously more rebasing problems for multitude of people than this
> patch or series does.

Please tone down the aggression a notch.

Philippe merely observed a trade-off.  We deal with such trade-offs all
the time.  When your reviewer challenges one, you consider it (unless
you did that already), then tell him what you concluded.

"Tu quoque" is not a proper reply to such an observation (or to anything
else for that matter).  When you have an issue with Philippe's patches,
address it in review of Philippe's patches.

Thank you.