Feature/1796 reflection #2960
Feature/1796 reflection #2960
Conversation
…opy and paste errors
…ists of strings for domains + parse options
…pace; renamed AbstractFunction -> AbstractReflectionFunction in order to avoid conflicts with SqlUtil::AbstractFunction
… gap in the API by implementing Function::getVariants()
include/qore/QoreNamespace.h
Outdated
| DLLEXPORT const QoreNamespace* get() const; | ||
|
|
||
| private: | ||
| class qore_namespace_namespace_iterator* priv; |
There was a problem hiding this comment.
why the PImpl idiom is used here?
also, let's use std::unique_ptr instead of explicitly allocating and deleting the pointer.
There was a problem hiding this comment.
why the PImpl idiom is used here?
to abstract the implementation of namespaces from the ABI - is there a better way to do this?
also, let's use std::unique_ptr instead of explicitly allocating and deleting the pointer.
we cannot do that without making the private implementation public, since the size of the object being allocated must be known at compile time
include/qore/QoreNamespace.h
Outdated
| //! allows local namespaces to be iterated | ||
| /** @since %Qore 0.8.13 | ||
| */ | ||
| class QoreNamespaceNamespaceIterator { |
There was a problem hiding this comment.
this class has implicit copy ctor and =operator, and the will do just a shallow copy and it will lead to undefined behavior, let's =delete them. Also let's =default the move ctor and =operator.
include/qore/QoreNamespace.h
Outdated
| DLLEXPORT QoreNamespaceNamespaceIterator(const QoreNamespace* ns); | ||
|
|
||
| //! destroys the iterator | ||
| DLLEXPORT ~QoreNamespaceNamespaceIterator(); |
There was a problem hiding this comment.
I like the rule where destructor is virtual, or class is final, so no one can inherit a class and take a pointer to its ancestor and then the destructor is not called, what do you think?
There was a problem hiding this comment.
ok I made all the destructors virtual
include/qore/QoreNamespace.h
Outdated
| class QoreNamespaceNamespaceIterator { | ||
| public: | ||
| //! creates the iterator | ||
| DLLEXPORT QoreNamespaceNamespaceIterator(const QoreNamespace* ns); |
There was a problem hiding this comment.
why are we taking pointer, if it cannot be null, why not reference?
include/qore/QoreNamespace.h
Outdated
| DLLEXPORT bool next(); | ||
|
|
||
| //! returns the namespace | ||
| DLLEXPORT const QoreNamespace* get() const; |
There was a problem hiding this comment.
why are we returning pointer, if it cannot be null, why not reference?
| else { | ||
| ++i; | ||
| } | ||
| return (i != ns->nsl.nsmap.end()); |
There was a problem hiding this comment.
this will (almost) always return true, right? because when it reaches end, it will go over again except for the case when begin==end. It's not documented that the iterator is cyclic, why do we need it?
There was a problem hiding this comment.
because this is how all Qore iterators work in Qore and in C++, for consistency's sake
refs #2984 fixed QUnit reporting for new tests
…reflection test fix
…ses, removed deprecated arguments from public APIs
… to target cmd
|
@sejvlond updated according to the latest feedback |
| delete i->second; | ||
| free(i->first); | ||
| delete i.second; | ||
| free(i.first); |
There was a problem hiding this comment.
this is really confusing, the class doesn't create any of the members, but it deallocates them and in the way that one was malloced, second was created with new. For better readability I would say either let the class to create them, so it's clear how they were allocated or use them as unique_ptr<char[]> and <unique_ptr>` so they will be deallocated automatically... what do u think?
There was a problem hiding this comment.
done except the char* issue, because this is allocated by the scanner, and changing this would require massive changes all over Qore - this needs to be changed, but in a major overhaul of memory management in the scanner
…per review comments
provides a new binary module for reflection called
reflection.Minor API and ABI changes were made; some modules will need to be patched to build against
developafter this change, and all modules will need to be rebuilt after the ABI change.Builds work with cmake & autotools, binary module documentation works with the
reflectionandastparsermodules now and has been integrated into the Qore documentation.Jenkins has been updated to run tests for binary modules delivered with Qore now.
Has been extensively tested with valgrind as well.