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

Update for breaking changes in scroll #114

Closed
wants to merge 1 commit into from

Conversation

willglynn
Copy link
Collaborator

This PR updates goblin for the breaking changes in m4b/scroll#45 and m4b/scroll#47. It's mostly stripping out Size/Units and adding &.

There's a few places where self -> &self meant that self.into() became &(*self.into()). This change results in copying different structs – sometimes structs which could have been #[derive(Copy)] but weren't, so I added that where necessary.

@raindev
Copy link
Contributor

raindev commented Jul 5, 2019

This would be great to get in. I've hit the incompatibility attempting to build goblin against the head of scroll.

@m4b
Copy link
Owner

m4b commented Jul 5, 2019

Yes I’d like to see this go in too, just a matter of someone doing the work I think. I don’t think there were any outstanding semantic issues with the change (and taking a reference for writers is probably the more intuitive approach).

Iirc, the state of the PR was that it just needed to be finished, and then the PRs in faerie and goblin could be updated to test the functionality.

Eg basically this thread: m4b/scroll#47

Iirc I think someone had a concern about annotated generics in turbofish position requiring a change, but because this is less common when pwrite-ing, not a super big deal ?

Otherwise @raindev afaik you should be able to build scroll master against goblin, since pwrite references haven’t landed; what issue are you bumping up against precisely ?

@m4b
Copy link
Owner

m4b commented Jul 5, 2019

Ah size associated type in SizeWith is also removed, I see. Well I think we could fix that separate to the PR and then maybe rebase @willglynn pr but that might have a ton of annoying conflicts

@andrewshadura
Copy link

Any updates?

@joshtriplett
Copy link

It would be nice to have goblin using an updated version of scroll, since scroll 0.9 uses older (pre-1.0.0) versions of proc_macro2, quote, and syn. Currently, using goblin alongside crates with newer proc macros will result in compiling multiple copies of those crates, which substantially increases compile time.

@m4b
Copy link
Owner

m4b commented Nov 1, 2019

Yup, will get that updated ASAP, probably this weekend, unless someone else wants to give it a shot ?

@m4b
Copy link
Owner

m4b commented Nov 1, 2019

And @joshtriplett to be clear this PR was for a PR that @willglynn prototyped for some semi major changes to scroll, namely having pwrite take references instead of ownership.

I’m still holding out hope it’ll be fixed up one day in scroll and we can land some changes, there were some outstanding issues however.

Anyway updating to crates.io scroll 0.10 should be relatively straightforward I believe

@andrewshadura
Copy link

Thanks @m4b.

@joshtriplett
Copy link

@m4b Ah, sorry for the mixup.

@m4b
Copy link
Owner

m4b commented Nov 3, 2019

Just heads up, goblin with latest scroll was published as 0.1.0; see #190

@m4b
Copy link
Owner

m4b commented Jan 31, 2021

closing as this is stale and and scroll has not merged change for pwrite to take by reference instead of owned version; all the scroll version changes should no longer be an issue, as we are on the latest scroll

@m4b m4b closed this Jan 31, 2021
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.

5 participants