-
Notifications
You must be signed in to change notification settings - Fork 649
Rust module API rework #1660
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
Conversation
3ebb648 to
9ea9756
Compare
gefjon
left a comment
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.