Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Aleksandar Markovic]

On May 8, 2019 11:53 PM, "Philippe Mathieu-Daudé" <address@hidden> wrote:
>
> Hi Richard, Aleksandar.
>
> On 5/8/19 4:32 PM, Richard Henderson wrote:
> > On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
> >>
> >> On May 8, 2019 2:19 AM, "Richard Henderson" <
address@hidden
> >> <mailto:address@hidden>> wrote:
> >>>
> >>>
> >>>
> >>
> >> This commit message doesnˊt explain the reason for the change, and why
is this
> >> an improvement. The underlyng reason for distingishing between
env_cpu and
> >> env_archcpu cases is not explained too.
> >
> > It's certainly explained in the preceeding patches that introduce those
functions.
> >
> > Are you suggesting that it is beneficial to copy-and-paste a common
block
> > explanation into 21 commit messages for each of target/foo/?
>
>
> *) Richard:
>
> I tried to put myself in Aleksandar shoes. I believe Aleksandar is
> worried about his MIPS maintainer duty, wanting to Ack-by this patch.
>
> It is true that out of the context of the series, it is hard to see what
> is the problem you try to solve.
>
> You could copy/paste the explanation you used previously,
> with s/$arch/mips/:
>
> "Cleanup in the boilerplate that each target must define."
>
> "Combined uses of CPU(mips_env_get_cpu()) were failures to use
> the more proper, ENV_GET_CPU macro, now replaced by env_cpu."
>
> Now to clearly understand this patch we still need to look at the
> previous two arch-generic patches
> - "cpu: Replace ENV_GET_CPU with env_cpu" and
> - "cpu: Introduce env_archcpu".
>
> Also, it is tedious to copy/paste the same explanation, but thinking of
> forks or stable branch that cherry-pick not all but some commits of a
> series, it might be useful.
>
> Another guess is Aleksandar might have looked at the series cover, which
> is not well explained as your v2:
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07635.html
> I think you mistakenly copied the v1 blurb instead of the v2 one.
>
> So at some point I can understand Aleksandar frustation.
>
>
> *) Aleksandar:
>
> This series fall under the "Overall Guest CPU cores (TCG)" section
> maintained by Richard and Paolo. I think you have to see this series as
> a whole to understand the benefits of it.
>
> With the same reasoning, I believe you shouldn't worry to not give your
> Ack if you don't feel comfortable.
>
> I think Richard sent this v3 to simply address comments raised by the
> previous reviewer during v1/v2, where there was some discussions: I took
> it as "this is the last round before getting merged" (unless someone
> object).
>
> It is hard to make everybody happy on a such big project, with so many
> areas, lines of code, people, culture, etc... I believe we all try to
> give our best, neither the commiters nor the reviewers are perfect, but
> slowly we help this project to improve :)
>
>
> Best regards,
>
> Phil.

Richard, Philippe,

A commit message along the line that Philippe put together would be OK.

I can talk about this commit only - if other submaintainers are fine with
empty commit messages in key files for their target, that is their
business. I am certainly opposed to any empty commit messages in MIPS
files, and please, Richard, include a decent commit message for this
commit. I don't think I am asking much.

Thanks,
Aleksandar