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.

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

Refactor host files into crates #54

Merged
merged 34 commits into from
Jul 31, 2024
Merged

Refactor host files into crates #54

merged 34 commits into from
Jul 31, 2024

Conversation

lukewilliamboswell
Copy link
Collaborator

@lukewilliamboswell lukewilliamboswell commented May 31, 2024

This PR separates the rust host files from the roc platform files, and adds a build.roc script.

The purpose of this change is to simplify the build process, and to have a clearer separation between these elements as this repository serves as an example of roc platforms.

Copy link
Sponsor Collaborator

@smores56 smores56 left a comment

Choose a reason for hiding this comment

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

Looks good! We may want to wait on merging for @Anton-4 to finish the roc-lang/roc#6873 investigation that is blocking the sister PR for basic-cli here: roc-lang/basic-cli#194

build.roc Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lukewilliamboswell
Copy link
Collaborator Author

  • Updated with changes to support the List refcounting performance fix.
  • Refactored Http to align closer to basic-cli and remove the glue generated types. Added roc_http and refactored the roc_host crate.
  • Fixed up and simplified the glue generation. Moved the glue-get.sh script to top level so it is easier to discover. The generated code still requires some manual clean-up but this is much simpler now.
  • Also includes a manual impl<T, E> RocRefcounted for RocResult<T, E> in roc_std that needs to be added into roc-lang/roc/crates/roc_std

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do formatting changes in a separate PR in the future.

Comment on lines +1729 to +1737
fn inc(&mut self) {
unimplemented!();
}
fn dec(&mut self) {
unimplemented!();
}
fn is_refcounted() -> bool {
true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems problematic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only matters if a list containing said type is passed between host and roc.

So if there are no effects of the sort List (Result Stream StremErr), doesn't matter.

Same witht he other case pointed out.

Comment on lines +1972 to +1982
impl roc_std::RocRefcounted for ReadResult {
fn inc(&mut self) {
unimplemented!();
}
fn dec(&mut self) {
unimplemented!();
}
fn is_refcounted() -> bool {
true
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was also generated by RustGlue.roc. I think we could remove it if we wanted to, because it's not used or needed. I think the implementation is just to satisfy the trait requirement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I'll try to remove them all

@Anton-4
Copy link
Collaborator

Anton-4 commented Jul 29, 2024

Looks like some of these are used...

@bhansconnect
Copy link
Collaborator

Looks like some of these are used...

Oh, I know whats happening. They type is being used in a RocResult. Which does use the refcounting impl. But still, the actual impl won't be called unless eventually the type is used in a list.

@bhansconnect
Copy link
Collaborator

So they are unused, but referenced by RocResult. So they can't be deleted.

@@ -0,0 +1,18 @@
TODO other crates
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just want to have this handled before we merge. I can probably get to it tomorrow but anyone is welcome to take it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can probably get to it tomorrow

I got deep down a segfault rabbit hole when updating the examples to basic-cli 0.13 :p

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lukewilliamboswell Can you explain what the purpose is of the roc_app and roc_fn crates and why we don't need those in basic-cli?

Copy link
Collaborator

@Anton-4 Anton-4 left a comment

Choose a reason for hiding this comment

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

Thank you very much @lukewilliamboswell!

@Anton-4 Anton-4 merged commit 856051c into main Jul 31, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants