Skip to content

Convert ZeroCopy to ouroboros. #208

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

Merged
merged 8 commits into from
Aug 16, 2021
Merged

Convert ZeroCopy to ouroboros. #208

merged 8 commits into from
Aug 16, 2021

Conversation

mordak
Copy link
Contributor

@mordak mordak commented Jul 6, 2021

This converts Capabilities to use ouroboros instead of the local ZeroCopy.

Because of all the magic that ouroboros self_referencing macro adds (CapabiltiesTryBuilder and friends) I found it was simpler to move the parsing bit into the Capabilities struct itself. I actually prefer this, since it keeps everything related to Capabilities in one place, but I understand if you prefer to keep everything related to parsing in parse.rs.

Anyway, if this approach for moving from ZeroCopy to ouroboros looks like a good approach, then I can do the rest of it similarly here, and ZeroCopy can go away, and we can close #125 .

Thoughts?


This change is Reviewable

@jonhoo
Copy link
Owner

jonhoo commented Jul 10, 2021

Yes, this looks very promising indeed! Also cc @joshua-maros just because they may be interested in an application of their crate :)

@someguynamedjosh
Copy link

Thanks for the mention, I always appreciate seeing something I made help someone out! As a side note, I've recently published 0.10.0 which fixes a potential source of undefined behavior, it should be a drop-in replacement for your use case.

mordak added 2 commits July 17, 2021 16:32
Use a helper function in `parse_many_into` to support parsing into
any container that implements Extend. Refactor Capabilities to use it.

Delete ZeroCopy and associated bits.

Move Flag into it's own module in types.
@mordak
Copy link
Contributor Author

mordak commented Jul 17, 2021

Pushed an update the switches Name and Fetch over to ouroboros. This lets us remove ZeroCopy.

Copy link
Owner

@jonhoo jonhoo 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 such an exciting change! I left some notes inline. Also just a heads up that I'll be out on vacation for a few weeks, so may take a while to get back to this.

assert_eq!(
names[0].attributes(),
first.attributes(),
Copy link
Owner

Choose a reason for hiding this comment

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

I think it'd be hugely valuable to continue to support indexing here if at all possible. I suspect all that'd be needed is to implement Index for Names and Fetches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I struggled to implement Index or Deref for this. The issue is the lifetime param on the Output associated type, since type Output = Name<'a>. The compiler wants the same lifetime on Output, but generic associated types are unstable.

We could probably get around this by replacing Names and Fetches with a generic container that takes a type param (we could call it.. ZeroCopy?), and then implement Deref or Index the same way that it is done for ZeroCopy, since the lifetime would be encapsulated in the generic type. Then Names is just ZeroCopy<Name<'_>>. When I try this I end up struggling against the constructor function signatures that ouroboros wants in the TryBuilder structure to initialize the self ref structure.

So instead I implemented a get() function, which trivially maps the underlying vector get() function and lets you just get an indexed member from the container. I personally kind of prefer this anyway, since Index can panic, but I get that the Index trait gives nice [] syntax.

If we want to impl Index or Deref (I think Deref would be better?), I am not sure how to make this work in the ouroboros context. I imagine that if it is doable then it is probably kind of simple, but I couldn't get it going.

Copy link
Owner

Choose a reason for hiding this comment

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

Oof, yeah, that's awkward. You can impl<'a> Index<usize> for &'a Fetches (playground), but it means &fetches[0] won't work, only (&fetches)[0], which is both hard to discover and ugly. I'm okay with fn get 👍

@mordak
Copy link
Contributor Author

mordak commented Jul 20, 2021

All of those comments look entirely reasonable! I'll be able to get it done soonish, and it should be gtg for when you get back from vacation.

Copy link
Owner

@jonhoo jonhoo 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 to me, thanks! Are there other things you wanted to change since this still stands as WIP?

assert_eq!(
names[0].attributes(),
first.attributes(),
Copy link
Owner

Choose a reason for hiding this comment

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

Oof, yeah, that's awkward. You can impl<'a> Index<usize> for &'a Fetches (playground), but it means &fetches[0] won't work, only (&fetches)[0], which is both hard to discover and ugly. I'm okay with fn get 👍

@mordak
Copy link
Contributor Author

mordak commented Aug 16, 2021

Looks good to me, thanks! Are there other things you wanted to change since this still stands as WIP?

Nope! Let me fix the title there... :)

@mordak mordak changed the title WIP: Convert ZeroCopy to ouroboros. Convert ZeroCopy to ouroboros. Aug 16, 2021
@jonhoo jonhoo merged commit 8841733 into jonhoo:master Aug 16, 2021
@jonhoo
Copy link
Owner

jonhoo commented Aug 16, 2021

Looks like the tarpaulin failure is due to xd009642/tarpaulin#756 btw.

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.

Move from ZeroCopy to the rental crate
3 participants