[Nepomuk] Re: SimpleResource rcgen redesign draft
Sebastian Trüg
trueg at kde.org
Thu Jul 28 11:35:35 CEST 2011
Actually Vishesh told me about virtual inheritance and now the problems
are all gone. The generated classes are simply sub-classes to
SimpleResource.
All that is left for me to fix is some compiler warnings about the order
of constructors.
Please find attached the patch for git master which does implement this.
Cheers,
Sebastian
On 07/28/2011 08:03 AM, Artem Serebriyskiy wrote:
> It is possible that I miss something really obvious. But could you
> please explain me in more details what is wrong if we do like this:
>
> 1) Make SimpleResource explicitly shared
> class SimpleResourcePrivate;
> class SimpleResource {
>
> .....
> private:
> QExplicitlySharedDataPointer<SimpleResourcePrivate> d;
> friend class WrapperBase;
> }
>
> and use detach in all non-const methods
>
> 2) Create a base class for all autogenerated wrappers.
> class WrapperBase
> {
> public constructors:
> WrapperBase( SimpleResource & )
>
> WrapperBas()
>
> private constructors:
> WrapperBase( QExplicitlySharedDataPointer<SimpleResourcePrivate> &
> resource )
>
> other methods:
> SimpleResource resource() const;
>
> protected:
> QExplicitlySharedDataPointer<SimpleResourcePrivate> m_resource;
>
> };
>
> and inherit all other classes from this class virtually if the do not
> have other base classes
>
> class InformationElement : virtual public WrapperBase
> {
> // Methods of InformatonElement here. They have access to m_resource to
> make changes
> };
>
> class TvSeries : InformationElement, SomeOtherBaseClass
> {
> // Methods of TvSeries here
> };
>
> And realizaton of
> SimpleResource WrapperBase::resource() const
> {
> return SimpleResource(m_resource);
> }
>
> On Wed, Jul 27, 2011 at 10:58 AM, Sebastian Trüg <trueg at kde.org
> <mailto:trueg at kde.org>> wrote:
>
> On 07/27/2011 08:14 AM, Artem Serebriyskiy wrote:
> >
> > 27.07.2011 3:12 пользователь "Christian Mollekopf"
> <chrigi_1 at fastmail.fm <mailto:chrigi_1 at fastmail.fm>
> > <mailto:chrigi_1 at fastmail.fm <mailto:chrigi_1 at fastmail.fm>>> написал:
> >>
> >> On Tue, 26 Jul 2011 20:57:46 +0200, Sebastian Trüg <trueg at kde.org
> <mailto:trueg at kde.org>
> > <mailto:trueg at kde.org <mailto: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 <mailto:trueg at kde.org>
> > <mailto:trueg at kde.org <mailto: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 <mailto:trueg at kde.org>
> > <mailto:trueg at kde.org <mailto: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 <mailto:trueg at kde.org> <mailto:trueg at kde.org
> <mailto: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?
>
> I think this results in the same multi-inheritance problem and results
> in very ugly memory maintenance. We cannot use protected member
> variables in the base classes since the compiler will not know which to
> use when inheriting from multiple classes.
>
> Thus, I think we are left with the only option that I found originally:
> plain wrappers without any convenience.
>
> Even code like
> Contact c;
> c.setFullname(...);
> SimpleResource res = c.resource();
> would require ugly pointer stuff which would look something like
>
> class Contact : public Role, public Party {
> public:
> Contact()
> : m_res(new SimpleResource()),
> m_deleteMe(true) {
> Role::setRes(m_res);
> Party::setRes(m_res);
> }
> ~Contact() {
> if(m_deleteMe) delete m_res;
> }
> SimpleResource resource() const {
> return *m_res;
> }
> void setRes(SimpleResource* res) {
> if(deleteMe) delete m_res;
> m_res = res;
> Role::setRes(res);
> Party::setRes(res);
> }
>
> private:
> SimpleResource* m_res;
> bool m_deleteMe;
> };
>
> Hm, maybe that is even doable....
>
> Cheers,
> Sebastian
>
> >>
> >> 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
> <http://quickgit.kde.org/?p=kde-runtime.git&a=blob&h=304db56a9aff5523a5672415550c49d4b7269c3c&hb=344806a2e378f3421399cad39a63f14a4428308b&f=nepomuk/services/storage/lib/nepomuk-simpleresource-rcgen.py>
> >
> <http://quickgit.kde.org/?p=kde-runtime.git&a=blob&h=304db56a9aff5523a5672415550c49d4b7269c3c&hb=344806a2e378f3421399cad39a63f14a4428308b&f=nepomuk/services/storage/lib/nepomuk-simpleresource-rcgen.py
> <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 <mailto:Nepomuk at kde.org>
> <mailto:Nepomuk at kde.org <mailto:Nepomuk at kde.org>>
> >> >> https://mail.kde.org/mailman/listinfo/nepomuk
> >> > _______________________________________________
> >> > Nepomuk mailing list
> >> > Nepomuk at kde.org <mailto:Nepomuk at kde.org>
> <mailto:Nepomuk at kde.org <mailto: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 <mailto:Nepomuk at kde.org> <mailto:Nepomuk at kde.org
> <mailto:Nepomuk at kde.org>>
> >> https://mail.kde.org/mailman/listinfo/nepomuk
> >
> >
> >
> > _______________________________________________
> > Nepomuk mailing list
> > Nepomuk at kde.org <mailto:Nepomuk at kde.org>
> > https://mail.kde.org/mailman/listinfo/nepomuk
> _______________________________________________
> Nepomuk mailing list
> Nepomuk at kde.org <mailto:Nepomuk at kde.org>
> https://mail.kde.org/mailman/listinfo/nepomuk
>
>
>
>
> --
> Sincerely yours,
> Artem Serebriyskiy
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 1.diff
Url: http://mail.kde.org/pipermail/nepomuk/attachments/20110728/86d4d6f7/attachment-0001.bat
More information about the Nepomuk
mailing list