-
Notifications
You must be signed in to change notification settings - Fork 162
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
Conversation
This would be great to get in. I've hit the incompatibility attempting to build goblin against the head of scroll. |
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 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 ? |
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 |
Any updates? |
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. |
Yup, will get that updated ASAP, probably this weekend, unless someone else wants to give it a shot ? |
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 |
Thanks @m4b. |
@m4b Ah, sorry for the mixup. |
Just heads up, goblin with latest scroll was published as |
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 |
This PR updates
goblin
for the breaking changes in m4b/scroll#45 and m4b/scroll#47. It's mostly stripping outSize
/Units
and adding&
.There's a few places where
self
->&self
meant thatself.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.