qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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
> 

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