-
Notifications
You must be signed in to change notification settings - Fork 110
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
Rust module API rework #1660
Rust module API rework #1660
Conversation
3ebb648
to
9ea9756
Compare
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.
- You still have the macros looking for known concrete types for columns marked
autoinc
. - I would really appreciate some comments and docs on the macros. In particular, listing some examples of expected inputs and outputs, and some explanation of why the different parts of the outputs are there and what parts of the runtime system they plug into.
crates/bindings/src/table.rs
Outdated
impl<T> BTreeIndexBoundsTerminator<T> for T {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for &T {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::Range<T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeFrom<&T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeFrom<T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeInclusive<&T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeInclusive<T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeTo<&T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeTo<T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeToInclusive<&T> {} | ||
impl<T> BTreeIndexBoundsTerminator<T> for ops::RangeToInclusive<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.
We need to discuss (on the proposal) whether we actually want to accept all combinations of T
and &T
.
1a7f7a1
to
a639404
Compare
a639404
to
47cf20d
Compare
Currently just blocked on #1704. |
1ffff46
to
3b6f8a2
Compare
I guess now it's just blocked on an implementation in ModuleHost |
9b6afb6
to
eff0e7e
Compare
The merge is just for testing - I'll rebase once #1697 actually merges. |
64d9963
to
965def9
Compare
@Centril do you know why this might be failing? In let index_id = Idx::index_id();
let args = b.get_args();
let (prefix, prefix_elems, rstart, rend) = args.args_for_syscall();
let iter = sys::datastore_btree_scan_bsatn(index_id, prefix, prefix_elems, rstart, rend)
.unwrap_or_else(|e| panic!("unexpected error from datastore_btree_scan_bsatn: {e}"));
TableIter::new(iter) It panics not at the lookup of the index id by name (searches the system tables), but in datastore_btree_scan_bsatn with "No such index" (searches |
275e18a
to
fd57613
Compare
gruhhh.... the tests are failing because the rust module now declares snake_case table names but the csharp module doesn't |
theoretically this is all tests passing, besides that |
maybe |
yeah, rust tests are all passing for me locally |
Reviewer's discretion whether this is an acceptable solution, I guess. |
Can you alter the C# module to define snake_case table names by changing |
Don't merge until we have a companion PR for private. |
ab53d8b
to
2fe3350
Compare
WIP - waiting on abi rework implementation.