[Nepomuk] Re: SimpleResource rcgen redesign draft
Artem Serebriyskiy
v.for.vandal at gmail.com
Thu Jul 28 08:03:11 CEST 2011
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> 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>> написал:
> >>
> >> On Tue, 26 Jul 2011 20:57:46 +0200, Sebastian Trüg <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>>
> >> >> 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>>
> >> >>>> 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>>
> >> >>>>>> 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
> >
> >> >> _______________________________________________
> >> >> 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
> >>
> >>
> >> --
> >> Using Opera's revolutionary email client: http://www.opera.com/mail/
> >> _______________________________________________
> >> 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
> > https://mail.kde.org/mailman/listinfo/nepomuk
> _______________________________________________
> Nepomuk mailing list
> Nepomuk at kde.org
> https://mail.kde.org/mailman/listinfo/nepomuk
>
--
Sincerely yours,
Artem Serebriyskiy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/nepomuk/attachments/20110728/90ef0323/attachment-0001.htm
More information about the Nepomuk
mailing list