[Nepomuk] Re: SimpleResource rcgen redesign draft
Christian Mollekopf
chrigi_1 at fastmail.fm
Wed Jul 27 01:11:25 CEST 2011
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.
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/
More information about the Nepomuk
mailing list