[Nepomuk] Re: SimpleResource rcgen redesign draft
Sebastian Trüg
trueg at kde.org
Wed Jul 27 08:58:31 CEST 2011
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
More information about the Nepomuk
mailing list