<br><br><div class="gmail_quote">On Tue, May 18, 2010 at 4:07 PM, Sebastian Trüg <span dir="ltr"><<a href="mailto:trueg@kde.org">trueg@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im">On 05/18/2010 12:30 PM, Vishesh Handa wrote:<br>
> 2. Don't we need to protect m_resources via a QMutex?<br>
><br>
><br>
> I'm currently blissfully unaware about concurrent programming. I assure<br>
> you I'm trying to learn, but till then could you take care of it?<br>
<br>
</div>ok, will do.<br>
<div class="im"><br>
> [3. Wouldn't shoudBeDeleted() make more sense in<br>
> ResourceManagerPrivate?]<br>
><br>
> :-/ It checks if a ResourceData should be deleted. I considered making<br>
> it static but this approach had less clutter. Could you please explain<br>
> why it should be in ResourceManagerPrivate?<br>
<br>
</div>I thought that conceptually it is the resourcemanager maintaining the<br>
resourcedata objects. So it should also be the one to decide when one<br>
has to be deleted.<br></blockquote><div><br>Okay. I've changed it.<br><br>The shouldBeDeleted function is currently called twice. Two times in Resource (destructor and operator==), and once in ResourceData::replaceWith.<br>
<br>- Vishesh Handa<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> As for ResourceManagerPrivate: this is just how it is done in KDE: it is<br>
> a convention to name private classes that way. You can look in KDE and<br>
> Qt code all over the place and find these private classes. Their main<br>
> purpose is to keep as much member variables and methods out of the main<br>
> class as possible to make it possible to add and remove those without<br>
> breaking binary compatibility.<br>
><br>
><br>
> Yea. I know. The "pimpl" idiom. :)<br>
><br>
> I will apply the patch and test it.<br>
><br>
> Okay. I'll do the same.<br>
><br>
> - Vishesh Handa<br>
><br>
><br>
> Cheers,<br>
> Sebastian<br>
><br>
> On 05/16/2010 04:11 PM, Vishesh Handa wrote:<br>
> > Cleaned up the code a little bit.<br>
> > - fixed a small bug in Resource::~Resource(). (my fault)<br>
> > - m_resources is now managed along with reference counting.<br>
> > - Added a function ResourceData::shouldBeDeleted() to avoid<br>
> duplication<br>
> > of code.<br>
> ><br>
> > I think the shouldBeDeleted() function should be in another patch.<br>
> ><br>
> > Btw, this code still hasn't been extensively tested. I'll do the<br>
> testing<br>
> > later on. I just though I should share the patch so that I can get<br>
> your<br>
> > comments.<br>
> ><br>
> > - Vishesh Handa<br>
><br>
><br>
</div></div></blockquote></div><br>