-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reworked RDFEntity interface and added SemanticEntity #37
Conversation
By the way, although this implementation differs from what I proposed previously, it fixes #31. The overhaul of RDFPropertyMap has removed the need for the workaround when deleting entries, and the SemanticEntity is the static alternative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review comments.
|
||
Internally, for every registered property an instance of `RegisteredPropertyBase` is stored in a vector of the SemanticEntity. Those objects simply provide an interface to `virtual std::string object() const = 0;` which is provided by the (templated) derived classes `RegisteredProperty`. The `RegisteredProperty`s in turn have to major modifications: | ||
|
||
1. They also provide a `bool isValid() const;` which is implemented differently for `std::shared_ptr`, in which case they return if the property/variable/member is not null. In case of non-shared-ptrs it always returns true. This method is used to check if a registered property should be skipped on iteration, as a nullptr is (IMHO) best represented by the absence of a triple. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect such a details description as code documentation not as general in the readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is well-documented, of course :D. Everything you read here is described in code-comments, too -- this is just a more structured explanation of how stuff works internally that can be read in one go.
Also, it is written beneath the header "Implementation details", so IMHO this is totally fine. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
include/sempr/entity/RDFEntity.hpp
Outdated
class TripleIterator_impl { | ||
friend class TripleIterator; | ||
protected: | ||
virtual ~TripleIterator_impl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange naming with *_impl and it implements nothing its just an interface.
Also the child's are named in this way :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that I want to return an iterator by value, and not a pointer to an iterator that points to some triples. The latter would force the user to write stuff like
(**it).subject;
or *(*it)->subject
and to delete the iterator in the end. If I'd just return a FooIterator, but the method signature in the RDFEntity class specifies a TripleIterator, the result would be a TripleIterator created from a FooIterator... not what I wanted. So my solution is to give the TripleIterator class a pointer to the actual implementation which has to be provided by the entity types that are RDF-iterable. The TripleIterator_impl
is the interface for this implementation, which we need for the TripleIterator to work correctly (virtual methods...).
I agree that the naming of the actual implementations [...]_impl
is a bit weird, but it simply follows the pattern. Do you have any suggestion how to improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think the TripleIterator
shall be renamed because its an abstract wrapper for the real iterators. It could be renamed to something like AbstractTripleIterator
or TripleIteratorWrapper
after these step the *_impl
could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'm on it.
namespace sempr { | ||
|
||
namespace rdf { | ||
namespace traits { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no better place to define the rdf traits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhhh it could be moved to a Utils or Traits Header in a (not existing) rdf subdir.
In general all the RDF specific entities could be moved in a entity/rdf
subdir. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle: Yes. But this leads to changes in everything that uses sempr, again... Maybe postpone it until we do a complete overhaul of the structure? I'm not really happy with the overall setup of directories and namespaces...
For now I'm fine with those traits being defined in the SemanticEntity.hpp, since only the SemanticEntity uses them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for now its fine.
src/entity/RDFPropertyMap.cpp
Outdated
|
||
const Triple& RDFPropertyMapIterator_impl::operator*() const | ||
{ | ||
static Triple t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why static?
t is changed by every call. It also could be non static and non reference to get a copy of the created object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because then the operator ->() const
wouldn't work anymore.
src/entity/RDFPropertyMap.cpp
Outdated
|
||
const Triple* RDFPropertyMapIterator_impl::operator->() const | ||
{ | ||
return &(**this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now it gets dirty. You will always return the address to the static local variable with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works. And it's a static variable. So what? ;D
I agree that this is a bit ugly. Maybe I was way too focused on creating an iterator that I favored the signatures over sensible implementation.
I've updated the TripleIterator to return triples by value instead of reference. I'm still open for naming-suggestions, but wouldn't mind to leave it as it is. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only found some out commeted stuff in a second review.
src/entity/RDFVector.cpp
Outdated
{ | ||
} | ||
|
||
// std::vector<Triple>::const_iterator RDFVector::begin() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this begin()
and end()
be removed? (also in the header file)
// RDFProperty<std::string> stringValue_; | ||
// RDFProperty<Person::Ptr> entityValue_; | ||
|
||
// TODO test composite value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still like to test it? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Composite values are... not so straight forward. I'll leave this for a future extension, just like vectors. So the answer is: No. ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you create a issue for this topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a small thing in the 'TripleIteratorWrapper'.
src/entity/RDFEntity.cpp
Outdated
{ | ||
return triples_.begin(); | ||
if (!impl_) | ||
throw std::exception(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't you trust yourself?
The impl_
point could be stored as const in the wrapper and the c'tor can check the null expr.
These functions could also be declared as inline (in a future release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... the TripleIteratorWrapper relies on the user providing him with a valid implementation. I don't know, it just felt right to check if I actually set it. Though this is probably a bit exaggerated... What do you want me to change? Just get rid of it? Or leave it as it is? Or do the check in the c'tor to fail more early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is to const the pointer and to have only one check in the c'tor. All other methods can than expect that the pointer is valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this? 4a6d8e4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR implements two major changes which build upon each other:
Of course there are still some limitations. One of them is the fact that you need to derive from SemanticEntity to make use of its features... This may be improved in the future.
Oh, and I'm also not quite satisfied with the RDFEntity-iterator-stuff. Both the RDFPropertyMap and the SemanticEntity do not store any actual
Triple
s , but the iterator constructs them on demand, without storing them. I use a static variable in the iterators dereference-operator for that, but that means that the returnedconst Triple&
is not really const! As soon as the iterator is incremented and dereferenced again, the static local variable changes its value... And when the iterator goes out of scope? 🙁So, be careful with that, I haven't really figured out how to do this properly. Any ideas?
(Take a look at this part of the code to see what I mean).