[Nepomuk] Nepomuk Core - Questions & Patches

[Vishesh Handa]

On Tue, May 18, 2010 at 4:07 PM, Sebastian Trüg <trueg at kde.org> wrote:

> On 05/18/2010 12:30 PM, Vishesh Handa wrote:
> >     2. Don't we need to protect m_resources via a QMutex?
> >
> >
> > I'm currently blissfully unaware about concurrent programming. I assure
> > you I'm trying to learn, but till then could you take care of it?
>
> ok, will do.
>
> >     [3. Wouldn't shoudBeDeleted() make more sense in
> >     ResourceManagerPrivate?]
> >
> > :-/ It checks if a ResourceData should be deleted. I considered making
> > it static but this approach had less clutter. Could you please explain
> > why it should be in ResourceManagerPrivate?
>
> I thought that conceptually it is the resourcemanager maintaining the
> resourcedata objects. So it should also be the one to decide when one
> has to be deleted.
>

Okay. I've changed it.

The shouldBeDeleted function is currently called twice. Two times in
Resource (destructor and operator==), and once in ResourceData::replaceWith.

- Vishesh Handa


> >     As for ResourceManagerPrivate: this is just how it is done in KDE: it
> is
> >     a convention to name private classes that way. You can look in KDE
> and
> >     Qt code all over the place and find these private classes. Their main
> >     purpose is to keep as much member variables and methods out of the
> main
> >     class as possible to make it possible to add and remove those without
> >     breaking binary compatibility.
> >
> >
> > Yea. I know. The "pimpl" idiom. :)
> >
> >     I will apply the patch and test it.
> >
> > Okay. I'll do the same.
> >
> > - Vishesh Handa
> >
> >
> >     Cheers,
> >     Sebastian
> >
> >     On 05/16/2010 04:11 PM, Vishesh Handa wrote:
> >     > Cleaned up the code a little bit.
> >     > - fixed a small bug in Resource::~Resource(). (my fault)
> >     > - m_resources is now managed along with reference counting.
> >     > - Added a function ResourceData::shouldBeDeleted() to avoid
> >     duplication
> >     > of code.
> >     >
> >     > I think the shouldBeDeleted() function should be in another patch.
> >     >
> >     > Btw, this code still hasn't been extensively tested. I'll do the
> >     testing
> >     > later on. I just though I should share the patch so that I can get
> >     your
> >     > comments.
> >     >
> >     > - Vishesh Handa
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/nepomuk/attachments/20100518/1d35c4df/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: removeProxyPatch4.diff
Type: text/x-patch
Size: 14150 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/nepomuk/attachments/20100518/1d35c4df/attachment-0001.diff