[Nepomuk] Re: SimpleResource rcgen redesign draft

Sebastian Trüg trueg at kde.org
Tue Jul 26 20:57:46 CEST 2011 

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.

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

More information about the Nepomuk mailing list