-
Notifications
You must be signed in to change notification settings - Fork 34
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
C++ driver UX improvements #541
C++ driver UX improvements #541
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
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.
self review
.circleci/config.yml
Outdated
@@ -460,7 +460,7 @@ commands: | |||
) | |||
tool/test/start-core-server.sh | |||
sleep 3 | |||
(cd test_assembly_cpp && LD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1) | |||
(cd test_assembly_cpp && DYLD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1) |
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 don't understand how the test CI assembly test passed. Would it not have reached the branch of the ||
?
https://app.circleci.com/pipelines/github/vaticle/typedb-driver/444/workflows/16c41bb4-012f-483b-8e37-c26d2ec3b787/jobs/4048
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 parentheses make the command run in a subprocess, I don't believe TEST_SUCCESS
is visible outside it. Remove the parens.
@@ -220,7 +220,7 @@ pub extern "C" fn value_group_drop(value_group: *mut ValueGroup) { | |||
} | |||
|
|||
#[no_mangle] | |||
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *const c_char { | |||
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *mut c_char { |
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.
@dmitrii-ubskii , Was there a reason these were const char* ? It stops me from calling string_free, but I do not how the other languages would have managed to free 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.
No, these should be *mut c_char
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.
These now also need to be marked as %newobject
in .i
so that the strings are GC'd correctly.
using pointer = T*; | ||
using reference = T&; | ||
using iterator_category = std::input_iterator_tag; | ||
|
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'm a little unsure of the difference_type, but found this on stack overflow
I want to introduce this, because when we try:
TypeDBIterable<...> i = ...;
std::vector v<SomeType>(i.begin(), i.end());
Would fail with
error: no matching constructor for initialization of 'std::vector<TypeDB::Database>'
note: candidate template ignored: substitution failure [with _InputIterator = TypeDB::Iterator<TypeDB::_native::DatabaseIterator, TypeDB::_native::Database, TypeDB::Database>]: no type named 'reference' in 'std::iterator_traits<TypeDB::Iterator<TypeDB::_native::DatabaseIterator, TypeDB::_native::Database, TypeDB::Database>>'
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_InputIterator __first, _InputIterator __last);
Now it fails with:
error: no matching constructor for initialization of 'std::vector<TypeDB::Database>'
note: candidate template ignored: requirement 'is_constructible<TypeDB::Database, TypeDB::Database &>::value' was not satisfied [with _InputIterator = TypeDB::Iterator<TypeDB::_native::DatabaseIterator, TypeDB::_native::Database, TypeDB::Database>]
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_InputIterator __first, _InputIterator __last);
Which makes sense if this methods needs a copy-constructor and we don't allow copying.
std::vector<Annotation> annotations; | ||
annotations.push_back(Annotation::key()); | ||
auto keys = collect(context.vars[matches[2].str()]->asThing()->getHas(context.transaction(), annotations)); | ||
auto keys = collect(context.vars[matches[2].str()]->asThing()->getHas(context.transaction(), {Annotation::key()})); |
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.
Use of Initializer_list
variant
@@ -55,6 +55,7 @@ class Driver { | |||
DatabaseManager databases; | |||
UserManager users; | |||
|
|||
static void initLogging(); |
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.
Added initLogging
to driver
private: | ||
VoidFuture setOwnsNative(Transaction& transaction, AttributeType* attributeType, AttributeType* overriddenTypeNative, const std::vector<Annotation>& annotations); | ||
ConceptIterable<AttributeType> getOwnsNative(Transaction& transaction, ValueType* valueType, const std::vector<Annotation>& annotations, Transitivity transitivity); | ||
|
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.
Realised I can hide these away altogether.
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.
Approved pending a couple comments.
@@ -220,7 +220,7 @@ pub extern "C" fn value_group_drop(value_group: *mut ValueGroup) { | |||
} | |||
|
|||
#[no_mangle] | |||
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *const c_char { | |||
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *mut c_char { |
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.
No, these should be *mut c_char
@@ -220,7 +220,7 @@ pub extern "C" fn value_group_drop(value_group: *mut ValueGroup) { | |||
} | |||
|
|||
#[no_mangle] | |||
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *const c_char { | |||
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *mut c_char { |
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.
These now also need to be marked as %newobject
in .i
so that the strings are GC'd correctly.
.circleci/config.yml
Outdated
@@ -460,7 +460,7 @@ commands: | |||
) | |||
tool/test/start-core-server.sh | |||
sleep 3 | |||
(cd test_assembly_cpp && LD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1) | |||
(cd test_assembly_cpp && DYLD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1) |
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 parentheses make the command run in a subprocess, I don't believe TEST_SUCCESS
is visible outside it. Remove the parens.
## Usage and product changes Add a few missing methods, and function variants for UX. ## Implementation * **Fixes** the `(DY)LD_LIBRARY_PATH` in the Mac circle assembly tests * Avoids using parentheses around commands in circleci config * UX changes: - Renames `drop()` methods to `deleteX` - Default arguments for QueryManager, - `initializer_list<Annotation>` variants for ConceptAPI * iterator traits, which don't do much but encounter more specific errors on unsupported usage of the iterator. * Hides away some static methods as regular methods in the CPP file * Updates `typedb_driver.i` to free the object returned by `concept_map_group_to_string` and `value_map_group_to_string`
Usage and product changes
Add a few missing methods, and function variants for UX.
Implementation
(DY)LD_LIBRARY_PATH
in the Mac circle assembly testsdrop()
methods todeleteX
initializer_list<Annotation>
variants for ConceptAPItypedb_driver.i
to free the object returned byconcept_map_group_to_string
andvalue_map_group_to_string