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

Add support for rust::Option<Box<T>> #1459

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

djmitche
Copy link

@djmitche djmitche commented Mar 5, 2025

While general support for Option<T> is difficult, Option<Box<T>> has a well-known layout due to the NPO, so we can replicate that layout in C++.


This is a very early draft!

@djmitche
Copy link
Author

djmitche commented Mar 5, 2025

I'd like a little feedback here. I've looked for the places where vec was referenced and tried to do similar things, although it turns out the implementation is closer to rust::Box. My understanding of what I'm doing is based on git grep and passing tests, and is not at all comprehensive.

  • Is this on the right track? Asking another way, do you look at this and thing "what is this nonsense" or "ok, it's a start"?
  • What issues, soundness or otherwise, have I not addressed?
    • One that comes to mind is ZSTs, which have non-zero pointer values that cannot be dereferenced. But, I think those are not allowed in rust::Box anyway?
  • On what should I model the methods for rust::Option -- std::Option or std::optional? or neither?
  • What else needs done? I know I need to write a .html page, and there are a few TODOs in the PR that I will get to.
    • I do need to add tests for Rust returning Option<Box<T>>.

@djmitche
Copy link
Author

Well, in the absence of any negative feedback, I guess I'll put this up for review :)

@djmitche djmitche marked this pull request as ready for review March 15, 2025 01:17
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.

1 participant