Re: [Development] Help reviewing my own changes to QSharedPointer
On quinta-feira, 7 de junho de 2012 09.43.40, Thiago Macieira wrote: > I've just had an idea to make this optimisation slightly more generic. I > need to experiment a little to see if it's worth it. If it is, I'll send a > replacement and let reviewers judge which one is better. I've done that experimentation now and I couldn't make it work. So I'm staging the change I had kept back. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
On quinta-feira, 7 de junho de 2012 06.47.11, lars.kn...@nokia.com wrote:
> >The new optimisation does not introduce "delete value" into the reference-
> >dropping code path. That is still:
> >
> > d->destroy()
> >which does:
> >void destroy() { destroyer(this); }
> > (destroyer is a function pointer)
> >
> >The optimisation simply changes what the pointed function does. Wiithout
> >it,
> >it will call another stored function pointer. With it, it will call
> >delete
> >directly.
>
> You're right. My brain probably stopped working shortly before midnight.
> Will review the remaining patches today as well.
I've just had an idea to make this optimisation slightly more generic. I need
to experiment a little to see if it's worth it. If it is, I'll send a
replacement and let reviewers judge which one is better.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Intel Sweden AB - Registration Number: 556189-6027
Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
signature.asc
Description: This is a digitally signed message part.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
On 6/7/12 12:55 AM, "ext Thiago Macieira"
wrote:
>On quarta-feira, 6 de junho de 2012 21.56.16, lars.kn...@nokia.com wrote:
>> >https://codereview.qt-project.org/26981-Optimisedeletion
>>
>> I might be wrong (getting tired), but I think this reintroduces the
>> problem with forward declared pointers that the first patch fixes.
>
>It's a different solution.
>
>The Qt 4 solution for this was to return false from a virtual function
>(the
>default implementation), which would require the caller to perform the
>deletion. The code was like this:
>
> if (!d->destroy())
> delete value;
>
>This function was called from deref(), which meant that where a reference
>could be dropped also required a full definition of the class.
Yes, that's what the first commit was fixing.
>
>The new optimisation does not introduce "delete value" into the reference-
>dropping code path. That is still:
>
> d->destroy()
>which does:
>void destroy() { destroyer(this); }
> (destroyer is a function pointer)
>
>The optimisation simply changes what the pointed function does. Wiithout
>it,
>it will call another stored function pointer. With it, it will call
>delete
>directly.
You're right. My brain probably stopped working shortly before midnight.
Will review the remaining patches today as well.
Cheers,
Lars
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
On quarta-feira, 6 de junho de 2012 21.56.16, lars.kn...@nokia.com wrote:
> >https://codereview.qt-project.org/26981-Optimisedeletion
>
> I might be wrong (getting tired), but I think this reintroduces the
> problem with forward declared pointers that the first patch fixes.
It's a different solution.
The Qt 4 solution for this was to return false from a virtual function (the
default implementation), which would require the caller to perform the
deletion. The code was like this:
if (!d->destroy())
delete value;
This function was called from deref(), which meant that where a reference
could be dropped also required a full definition of the class.
The new optimisation does not introduce "delete value" into the reference-
dropping code path. That is still:
d->destroy()
which does:
void destroy() { destroyer(this); }
(destroyer is a function pointer)
The optimisation simply changes what the pointed function does. Wiithout it,
it will call another stored function pointer. With it, it will call delete
directly.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Intel Sweden AB - Registration Number: 556189-6027
Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
signature.asc
Description: This is a digitally signed message part.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
On 6/6/12 6:53 PM, "ext Thiago Macieira" wrote: >Cf. thread: Maintainer "TrustMes" >http://lists.qt-project.org/pipermail/development/2012-April/002930.html >http://comments.gmane.org/gmane.comp.lib.qt.devel/2953 > >I need help again reviewing my own changes. Since I am the maintainer of >the >module that the changes are going to, there's no one else to call upon. I >asked Lars and he said he can't safely review them. I'm asking the >community >here to help out. > >The first two changes are extremely important and must be done before 5.0: > >https://codereview.qt-project.org/26974 - Remove "delete value" >https://codereview.qt-project.org/26975 - Remove virtuals from the private > >The next few changes are cleanups: >https://codereview.qt-project.org/26977 - Merge two private classes >https://codereview.qt-project.org/26978 - Merge the private to the public >https://codereview.qt-project.org/26979 - Use the copy & swap trick > >Then there are some optimisations: >https://codereview.qt-project.org/26980 - Optimise the constructor Got until the change above and gave up for today at the next one. Will get back to it tomorrow. >https://codereview.qt-project.org/26981 - Optimise deletion I might be wrong (getting tired), but I think this reintroduces the problem with forward declared pointers that the first patch fixes. Cheers, Lars > >One new feature (we should leave for 5.1): >https://codereview.qt-project.org/26982 - Add create() with variadic >arguments >and perfect forwarding > >And one unit test: >https://codereview.qt-project.org/27043 - lambda as custom deleter > >-- >Thiago Macieira - thiago.macieira (AT) intel.com > Software Architect - Intel Open Source Technology Center > Intel Sweden AB - Registration Number: 556189-6027 > Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden >___ >Development mailing list >Development@qt-project.org >http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
On 6/6/12 7:33 PM, "ext Thiago Macieira" wrote: >On quarta-feira, 6 de junho de 2012 17.08.29, lars.kn...@nokia.com wrote: >> Can't remember that you asked me. I can certainly review them if you >>don't >> have anyone else. I'll do so later tonight. > >I asked on IRC how you felt about reviewing QSharedPointer changes and >you >said you'd rather not do it. lars: btw, how comfortable are you reviewing QSharedPointer changes? thiago: depends on the change... ;-) I certainly can/will have a look. Will tell when I want another pair of eyes :) Cheers, Lars ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
On quarta-feira, 6 de junho de 2012 17.08.29, lars.kn...@nokia.com wrote: > Can't remember that you asked me. I can certainly review them if you don't > have anyone else. I'll do so later tonight. I asked on IRC how you felt about reviewing QSharedPointer changes and you said you'd rather not do it. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Help reviewing my own changes to QSharedPointer
Can't remember that you asked me. I can certainly review them if you don't have anyone else. I'll do so later tonight. Cheers, Lars On 06.06.12 18:54 ext Thiago Macieira wrote: Cf. thread: Maintainer "TrustMes" http://lists.qt-project.org/pipermail/development/2012-April/002930.html http://lists.qt-project.org/pipermail/development/2012-April/002930.html I need help again reviewing my own changes. Since I am the maintainer of the module that the changes are going to, there's no one else to call upon. I asked Lars and he said he can't safely review them. I'm asking the community here to help out. The first two changes are extremely important and must be done before 5.0: http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Remove "delete value" http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Remove virtuals from the private The next few changes are cleanups: http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Merge two private classes http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Merge the private to the public http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Use the copy & swap trick Then there are some optimisations: http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Optimise the constructor http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Optimise deletion One new feature (we should leave for 5.1): http://lists.qt-project.org/pipermail/development/2012-April/002930.html - Add create() with variadic arguments and perfect forwarding And one unit test: http://lists.qt-project.org/pipermail/development/2012-April/002930.html - lambda as custom deleter -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
