Re: [Qemu-devel] [PATCH v3 1/1] configure: Define target access alignment in configure

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

On Tue, Jul 30, 2019 at 10:06 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 30 Jul 2019 at 21:00, Aleksandar Markovic
> <aleksandar.m.mail@gmail.com> wrote:
> >
> > On Thu, Jul 25, 2019 at 3:25 AM <tony.nguyen@bt.com> wrote:
> >
> > > Rename ALIGNED_ONLY to TARGET_ALIGNED_ONLY for clarity and move
> > > defines out of target/foo/cpu.h into configure, as we do with
> > > TARGET_WORDS_BIGENDIAN, so that it is always defined early.
> > >
> > > Also, poison the symbol in include/exec/poison.h to prevent use in
> > > common code.
> > >
> > >
> > Hi, Tony.
> >
> > Thanks for this new version.
> >
> > When one mentions "also" in the commit message, this is a kind of strong
> > indication that the patch should be split into two patches.
> >
> > So, could you please consider moving "poison" part into a separate patch?
>
> I think in this case the issue is more in the phrasing of the commit
> message rather than the patch composition. The patch is
> introducing TARGET_ALIGNED_ONLY as something defined
> by configure, and the correct status for that macro is "needs to
> be in poison.h"; having an intermediate state where the macro
> exists but isn't poisoned isn't really a logical split of the work.
> (The previous ALIGNED_ONLY didn't have so much need to be
> poisoned because it was defined in cpu.h and so the only way to
> expect to get it was by pulling in cpu.h.)
>
>
Yes, I would say the same now that I am looking at the change more
carefully.

But then, repeating what you said, something like "poisoning is needed"
(together with an explanation "why") should be in the commit message.
"Also" doesn't not fit well here.

Aleksandar


> thanks
> -- PMM
>