Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Split tuples, arrays and the C API off from the records file and refactored them #872

Merged
merged 3 commits into from
Dec 22, 2020
Merged

Split tuples, arrays and the C API off from the records file and refactored them #872

merged 3 commits into from
Dec 22, 2020

Conversation

Kixiron
Copy link
Contributor

@Kixiron Kixiron commented Dec 17, 2020

  • Made differential_datalog/records.rs into a module
  • Pulled the C API out of it, refactored a bit and put it into a feature gated file
  • Rewrote the tuple macro, added 1-tuples and moved it to a file
  • Refactored the array macro and moved it to a file
  • Added some utility functions to the Record enum

ryzhyk
ryzhyk approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@ryzhyk ryzhyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks for all the cleanups! I fixed regressions and rebased. In addition to using a 1.48 feature your code is using some other post 1.41 features, so I decided to advance the earliest supported version of Rust to 1.47. I will push that change to this branch once I'm done uploading a new 1.47-based docker image (my upload speed sucks), and testing with it.

rust/template/differential_datalog/record/arrays.rs Outdated
$(
impl<T: FromRecord> FromRecord for [T; $length] {
fn from_record(val: &Record) -> Result<Self, String> {
Vec::from_record(val)?.try_into().map_err(|_| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires Rust 1.48 (https://doc.rust-lang.org/std/primitive.array.html#impl-TryFrom%3CVec%3CT%3E%3E).
I will roll this back to the old implementation.

@ryzhyk
Copy link
Contributor

ryzhyk commented Dec 21, 2020

PS. I noticed that git diff is useless when renaming files is involved. It'd make reviewing a lot easier if changes that involve renaming files were submitted as separate commits to make it easier to identify what's actually changed in the code.

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 21, 2020

I'm sorry about that, I thought I had split it into a separate commit

@ryzhyk ryzhyk force-pushed the rework-macros branch 2 times, most recently from f7dcc30 to e476cac Compare December 22, 2020 07:13
@ryzhyk
Copy link
Contributor

ryzhyk commented Dec 22, 2020 •
edited
Loading

C API in record/c_api.rs does not get exported in the generated .so file, most likely due to this bug: rust-lang/rust#50007. It does get exported if I merge the C API back into record/mod.rs. Any objections to this change?

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 22, 2020

That's very unfortunate, no objections from me

Kixiron and others added 3 commits December 22, 2020 12:08
@Kixiron @ryzhyk
Split tuples, arrays and the C API off from the records file and refa…
625c3e6 
…ctored them
@ryzhyk
Upgrade minimal supported version of Rust to 1.47.
b18c6f3 
We have so far maintained compatibility with Rust 1.41+.  Newer versions
have more and more features that we'd like to use, as well as some that
break backwards compatibility.  We therefore upgrade our Rust dependency
to 1.47.

Some earlier versions might still work, but 1.47.0 is going to be the
earliest "officially" supported one.
@ryzhyk
Move record::c_api back in the main record module.
05a471e 
After `record.rs` was split into multiple modules, the C API disappeared
from the compiled .so library.  This is likely due to this compiler bug:
rust-lang/rust#50007

As a workaround until the bug is fixed, we move the API from `record/c_api.rs`
to `record/mod.rs`.
@ryzhyk ryzhyk merged commit 6293df0 into vmware:master Dec 22, 2020
@Kixiron Kixiron deleted the rework-macros branch December 23, 2020 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzhyk ryzhyk ryzhyk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Kixiron @ryzhyk