[Nepomuk] Re: SimpleResource rcgen redesign draft
Artem Serebriyskiy
v.for.vandal at gmail.com
Wed Jul 27 08:14:28 CEST 2011
27.07.2011 3:12 пользователь "Christian Mollekopf" <chrigi_1 at fastmail.fm>
написал:
>
> On Tue, 26 Jul 2011 20:57:46 +0200, Sebastian Trüg <trueg at kde.org> wrote:
>
> > On 07/26/2011 05:06 PM, Christian Mollekopf wrote:
> >> On Tue, 26 Jul 2011 13:26:20 +0200, Sebastian Trüg <trueg at kde.org>
> >> wrote:
> >>
> >>> On 07/25/2011 09:43 PM, Christian Mollekopf wrote:
> >>>> On Mon, 25 Jul 2011 21:20:45 +0200, Sebastian Trüg <trueg at kde.org>
> >>>> wrote:
> >>>>
> >>>>> On 07/25/2011 07:52 PM, Christian Mollekopf wrote:
> >>>>>> On Mon, 25 Jul 2011 18:30:05 +0200, Sebastian Trüg <trueg at kde.org>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Now that the SimpleResource rcgen is in git master[1] some people
> >>>>>>> used
> >>>>>>> it (mostly for Akonadi feeders and the web extractor framework)
and
> >>>>>>> found it to be a bit strange.
> >>>>>>>
> >>>>>>> This is true. Current usage is as follows:
> >>>>>>>
> >>>>>>> SimpleResource r;
> >>>>>>> NCO::PersonContact(&r).setFullname("foobar");
> >>>>>>>
> >>>>>>> The reason for this is simple: due to multi-inheritance problems I
> >>>>>>> cannot derive the generated classes from SimpleResource. Thus, we
> >>>>>>> need
> >>>>>>> wrappers.
> >>>>>>>
> >>>>>>> Now some ideas were floating around and I would like to settle on
> >>>>>>> one
> >>>>>>> better solution.
> >>>>>>>
> >>>>>>> One idea would be to have a hybrid thing where you could do this:
> >>>>>>>
> >>>>>>> NCO::PersonContact pc;
> >>>>>>> pc.setFullname("foobar");
> >>>>>>> SimpleResource r = rc.resource();
> >>>>>>>
> >>>>>>> and this:
> >>>>>>>
> >>>>>>> SimpleResource r;
> >>>>>>> NCO::PersonContact pc(r);
> >>>>>>> pc.setFullname("foobar");
> >>>>>>> r = pc.resource();
> >>>>>>>
> >>>>>>> (The last line would be required since all SimpleResources would
be
> >>>>>>> copies.)
> >>>>>>
> >>>>>> Why can't you just take the reference to r, and the last copy
> >>>>>> wouldn't
> >>>>>> be
> >>>>>> needed?
> >>>>>> Of course the accessor is still needed for the NCO::PersonContact
> >>>>>> pc;
> >>>>>> usecase.
> >>>>>
> >>>>> I cannot use refs for security reasons. Imagine someone returns a
> >>>>> generated class from a method after putting in a ref to a local
> >>>>> SimpleRes. Anyway, SimpleRes is implicitly shared.
> >>>>
> >>>> Thats the same as with pointers, and it's c++ after all, developers
> >>>> have
> >>>> to think a bit.
> >>>> I just think that forgetting r = pc.resource() is more likely than
> >>>> returning a generated class
> >>>> containing a local reference, because that is a problem you should be
> >>>> used
> >>>> to as c++ developer
> >>>> (although a bit nasty I admit) and
> >>>>
> >>>> SimpleResource r;
> >>>> NCO::PersonContact pc(r);
> >>>> pc.setFullname("foobar");
> >>>>
> >>>> doesn't really look "wrong" at all (unless you have a close look at
> >>>> the
> >>>> header).
> >>>
> >>> Actually I think this only looks quite correct to you since you have
> >>> been using the old rcgen. If you look at pretty much all other Qt/KDE
> >>> APIs you will see that this is a typical copy situation.
> >>>
> >>
> >> Except if you think the class is a wrapper.
> >> The copying seems to me more java style.
> >
> > Well, the problem is that we want wrapping and a member
SimpleResource...
> >
> >>> And I would really prefer not to use references. Aside from the
> >>> unwritten policy in Qt and KDE code to not do it it is so very error
> >>> prone since people tend to overlook the little "&". That is also why I
> >>> chose to use a pointer in the original version. It forces one to add
> >>> the
> >>> "&" ones self and, thus, think about what one it doing.
> >>>
> >>
> >> Ok, my intuition is different, also before using the old rcgen.
> >> Also I happen to like references =)
> >
> > They are very powerful indeed.
> >
> >> Further, just disable the copy constructor in the wrapper classes and
> >> all
> >> problems are gone (I reckon).
> >
> > We could do that but that would not solve the problem with the
> > reference. We want to allow both
> > SimpleResource res;
> > Contact c(res);
> > and
> > Contact c;
> > c.resource();
> > That means that we have to store a pointer and convert the reference to
> > a pointer (ugly!) since a reference member needs to be initialized in
> > the constructor. Thus, the second case would mean to create a
> > SimpleResource on the heap which leads to memory management trouble.
What if instead of storing SimpleResource (or ref or pointer) store ptr to
SimpleResource::Private( and make it less private, of course ) ? This way
Contact c(r); will share with r it's privates, Contact c; will create new
private and in all cases c.resource(); will detach a private and create a
new SimpleResource with detached private copy?
>
> Ok, you're right, I completely forgot about the second usecase =)
>
> >
> > Cheers,
> > Sebastian
> >
> >>>> Anyways, no strong preference from my side.
> >>>
> >>> So not get me wrong: I like the idea of making it fast. But I am also
> >>> very concerned about usability of the API. I would rather have it be
> >>> pretty than super-fast. And in the end the overhead of copying a few
> >>> pointers or primitives is nothing compared to the actual calling of
> >>> storeResources. ;)
> >>
> >> Agreed, it's not really a performance issue.
> >> I just don't like that last "r = pc.resource();", which is imo
> >> unnecessary, error prone and
> >> feels to me like java (where you never modify the object itself but
> >> always
> >> get a copy back).
> >>
> >> Anyways, it's really just my personal opinion/preference, and I'm fine
> >> with either way.
> >> Especially since you did a good job in providing a nice api so far =)
> >>
> >> Cheers,
> >> Christian
> >>
> >>>
> >>> Cheers,
> >>> Sebastian
> >>>
> >>>> Cheers
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>> We could go on like so:
> >>>>>>>
> >>>>>>> r.addProperty(...);
> >>>>>>> pc = r;
> >>>>>>> pc.setResource(r);
> >>>>>>>
> >>>>>>> The only disadvantage I see here might be a slight performance
> >>>>>>> overhead
> >>>>>>> due to the copying of the resources.
> >>>>>>>
> >>>>>>> Opinions?
> >>>>>>
> >>>>>> Additionally it might be nice to have an accessor to the uri of the
> >>>>>> SimpleResource directly in the wrapper,
> >>>>>> and the << operator for the graph.
> >>>>>>
> >>>>>> So we would go from this:
> >>>>>>
> >>>>>> Nepomuk::SimpleResource iconRes;
> >>>>>> Nepomuk::NAO::FreeDesktopIcon icon(&iconRes);
> >>>>>> icon.setIconNames( QStringList() << iconName );
> >>>>>> graph << iconRes;
> >>>>>> res.setProperty( Soprano::Vocabulary::NAO::prefSymbol(),
> >>>>>> iconRes.uri() );
> >>>>>>
> >>>>>> to this:
> >>>>>>
> >>>>>> Nepomuk::NAO::FreeDesktopIcon icon;
> >>>>>> icon.setIconNames( QStringList() << iconName );
> >>>>>> graph << icon;
> >>>>>> res.setProperty( Soprano::Vocabulary::NAO::prefSymbol(),
> >>>>>> icon.uri()
> >>>>>> );
> >>>>>
> >>>>> nice.
> >>>>>
> >>>>> Cheers
> >>>>> Sebastian
> >>>>>
> >>>>>> Cheers,
> >>>>>>
> >>>>>> Christian
> >>>>>>
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>> Sebastian
> >>>>>>>
> >>>>>>> [1]
> >>>>>>>
http://quickgit.kde.org/?p=kde-runtime.git&a=blob&h=304db56a9aff5523a5672415550c49d4b7269c3c&hb=344806a2e378f3421399cad39a63f14a4428308b&f=nepomuk/services/storage/lib/nepomuk-simpleresource-rcgen.py
> >> _______________________________________________
> >> Nepomuk mailing list
> >> Nepomuk at kde.org
> >> https://mail.kde.org/mailman/listinfo/nepomuk
> > _______________________________________________
> > Nepomuk mailing list
> > Nepomuk at kde.org
> > https://mail.kde.org/mailman/listinfo/nepomuk
>
>
> --
> Using Opera's revolutionary email client: http://www.opera.com/mail/
> _______________________________________________
> Nepomuk mailing list
> Nepomuk at kde.org
> https://mail.kde.org/mailman/listinfo/nepomuk
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/nepomuk/attachments/20110727/02204d45/attachment.htm
More information about the Nepomuk
mailing list