From: Jianjun Duan <duanj@linux.vnet.ibm.com>
To: Halil Pasic <pasic@linux.vnet.ibm.com>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: veroniabahaa@gmail.com, peter.maydell@linaro.org,
mdroth@linux.vnet.ibm.com, mst@redhat.com, quintela@redhat.com,
mark.cave-ayland@ilande.co.uk, qemu-devel@nongnu.org,
mreitz@redhat.com, blauwirbel@gmail.com, amit.shah@redhat.com,
qemu-ppc@nongnu.org, kraxel@redhat.com, kwolf@redhat.com,
dmitry@daynix.com, pbonzini@redhat.com, rth@twiddle.net,
leon.alrae@imgtec.com, aurelien@aurel32.net,
david@gibson.dropbear.id.au
Subject: Re: [Qemu-devel] [Qemu-ppc] [QEMU PATCH v9 2/3] migration: migrate QTAILQ
Date: Mon, 31 Oct 2016 11:25:05 -0700 [thread overview]
Message-ID: <ea1cf737-388e-7790-2e4b-ed92cdd72f5e@linux.vnet.ibm.com> (raw)
In-Reply-To: <8565b420-bb18-0907-8433-9f66285f7d80@linux.vnet.ibm.com>
On 10/31/2016 11:01 AM, Halil Pasic wrote:
>
>
> On 10/31/2016 06:32 PM, Jianjun Duan wrote:
>>>>> I think this got overly complicated. Here is a little patch on
>>>>>>>>>> top of your stuff which gets rid of 15 lines and IMHO simplifies
>>>>>>>>>> things quite a bit. What do you think?
>>>>>>>>>>
>>>>>>>>>> It is based on/inspired by Dave's proposal with the dummy stuff.
>>>>>>>>>>
>>>>>>>>>> Did not address the typos though.
>>>>>> It is unlikely the definition of QTAILQ will change, so hard-coded
>>>>>> value probably was the most simple. Now that we want to address the
>>>>>> potential changes, I think my code will deal with future changes better.
>>>>
>>>> Please elaborate in what way does your version deal better with future
>>>> changes? IMHO the version with my patch applied covers all the corners
>>>> your original code covers but it is without any doubt more concise and
>>>> in my opinion also more straight-forward.
>> I don't use the internals of head and entry structures if there are
>> access macro around. Also I didn't use pointer type cast. I don't think
>> pointer cast is more straightforward.
>
>
> Internals is quite relative since the stuff is part of the header
> and there is no indication that it's not part of the API. About
> pointer type casts I do not understand what you mean by that:
> my understanding is that we both do casts to a pointer type. And
> I do think it is more straightforward than defining a macro for the
> offset for each link-pointer and then basically play the field_at_offset
> game twice: once to pin point the struct holding all the links, and
> once again to pinpoint the individual links.
>
It is more concise but not more straightforward for me.
> As you see I do not believe you that your version is more robust.
> If you want to convince me _give me a remotely reasonable patch_ which
> beaks my version but not yours.
>
> The straight-forwardness is probably a matter of taste, and that's
> why I used IMHO. I'm not sure if I understood you correctly, do
> you think that the current version of the code is more readable
> that what I have proposed? If you do, I doubt there is a rational
> way to settle this taste dispute. If the majority of the people
> says your approach is more straight-forward then I apologize.
>
I agree it is a matter of taste. With current code both versions will
not break. But if somebody changes the name of the var you directly
accessed, your code will break. Of course I don't expect somebody to
just do that.
It is indeed in the header file and we expect people to change all at
once if any change is done. I used this argument for hard encoding and
it is not taken.
Thanks,
Jianjun
> Cheers,
> Halil
>
next prev parent reply other threads:[~2016-10-31 18:25 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-27 22:28 [Qemu-devel] [QEMU PATCH v9 0/3] migration: migrate QTAILQ Jianjun Duan
2016-10-27 22:28 ` [Qemu-devel] [QEMU PATCH v9 1/3] migration: extend VMStateInfo Jianjun Duan
2016-10-27 22:28 ` [Qemu-devel] [QEMU PATCH v9 2/3] migration: migrate QTAILQ Jianjun Duan
2016-10-28 19:06 ` Dr. David Alan Gilbert
2016-10-28 19:46 ` Jianjun Duan
2016-10-31 13:10 ` Halil Pasic
2016-10-31 17:10 ` [Qemu-devel] [Qemu-ppc] " Jianjun Duan
2016-10-31 17:21 ` Halil Pasic
2016-10-31 17:32 ` Jianjun Duan
2016-10-31 18:01 ` Halil Pasic
2016-10-31 18:25 ` Jianjun Duan [this message]
2016-10-31 19:02 ` Halil Pasic
2016-11-01 15:02 ` [Qemu-devel] " Paolo Bonzini
2016-11-01 15:47 ` Halil Pasic
2016-10-27 22:28 ` [Qemu-devel] [QEMU PATCH v9 3/3] tests/migration: Add test for QTAILQ migration Jianjun Duan
2016-10-28 13:57 ` [Qemu-devel] [QEMU PATCH v9 0/3] migration: migrate QTAILQ no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ea1cf737-388e-7790-2e4b-ed92cdd72f5e@linux.vnet.ibm.com \
--to=duanj@linux.vnet.ibm.com \
--cc=amit.shah@redhat.com \
--cc=aurelien@aurel32.net \
--cc=blauwirbel@gmail.com \
--cc=david@gibson.dropbear.id.au \
--cc=dgilbert@redhat.com \
--cc=dmitry@daynix.com \
--cc=kraxel@redhat.com \
--cc=kwolf@redhat.com \
--cc=leon.alrae@imgtec.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=mdroth@linux.vnet.ibm.com \
--cc=mreitz@redhat.com \
--cc=mst@redhat.com \
--cc=pasic@linux.vnet.ibm.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=quintela@redhat.com \
--cc=rth@twiddle.net \
--cc=veroniabahaa@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).