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 Deserialize impl for CStr #811

Merged
merged 2 commits into from
Mar 9, 2017
Merged

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Mar 9, 2017

Fixes #810.

I've scoped the impl under feature="unstable" for now (don't know what the policy for that feature is), but this should possibly not be merged until rust-lang/rust#40380 lands and fully stabilizes CString::into_boxed_c_str. At that point, feature="unstable" should obviously be removed.

@dtolnay
Copy link
Member

dtolnay commented Mar 9, 2017

The method is in nightly but you are missing an underscore. 😉

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 9, 2017

Haha, I am indeed. Fix coming shortly.

@jonhoo jonhoo force-pushed the deserialize-cstr branch from 344f027 to d10b733 Compare March 9, 2017 04:52
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 9, 2017

I'm not entirely sure how to run the tests including the unstable ones though?

cargo test --features "serde/unstable

builds, but doesn't run them, and

cargo test --features "unstable"

errors out saying that there's no such feature?

@jonhoo jonhoo force-pushed the deserialize-cstr branch from d10b733 to 975bb07 Compare March 9, 2017 04:57
@jonhoo jonhoo force-pushed the deserialize-cstr branch from 975bb07 to 0c2fed4 Compare March 9, 2017 04:57
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 9, 2017

And

rustup run nightly cargo t --features "unstable-testing"

comes back with a whole host of errors from compiletest, which I believe are unrelated to my changes.

@dtolnay
Copy link
Member

dtolnay commented Mar 9, 2017

Yep the unstable testing situation was pretty messed up. I think I fixed it in dd6989d.

@dtolnay
Copy link
Member

dtolnay commented Mar 9, 2017

The compiletest setup is unfortunately really fragile in how it detects and links dependencies. Try the following:

cd test_suite/deps
cargo clean
cargo +nightly build
cd ..
cargo +nightly test --features unstable

@dtolnay dtolnay merged commit 6750fda into serde-rs:master Mar 9, 2017
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 9, 2017

Oooh, I completely missed the +TOOLCHAIN feature release!

cargo clean didn't work inside deps/:

test_suite/deps $ cargo clean
error: package `/home/jon/dev/others/serde/serde_derive/Cargo.toml` is a member of the wrong workspace
expected: /home/jon/dev/others/serde/test_suite/deps/Cargo.toml
actual:   /home/jon/dev/others/serde/Cargo.toml

The other commands worked fine though, and all tests passed!

I guess technically we should now have a way of monitoring when into_boxed_c_str stabilizes, and then remove the unstable feature flag... Or maybe it's fine if I just monitor rust-lang/rust#40380 and submit another PR when that day comes.

@dtolnay
Copy link
Member

dtolnay commented Mar 9, 2017

Looks like the workspace issue is fixed in nightly: cargo +nightly clean should work.

I don't think we need an issue to track stabilizing this. Typically for every major release of Serde we decide what the minimum supported Rust version should be and then we remove as many of the #[cfg(feature = "unstable")] as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants