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

[Aleksandar Markovic]

>

 

Markus wrote:

 

> However, I find the common pattern applied here
>
>     case 3: /* ERL */
>         /* If EU is set, always unmapped */
>         if (eu) {
>             return 0;
>         }
>
> more readable ...
>
>

 

 I am going to do it this way in v5.

Thanks,

Aleksandar

>
> ... 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!