rust-lang-nursery / lazy-static.rs

A small macro for defining lazy evaluated static variables in Rust.
Apache License 2.0
1.91k stars 111 forks source link

Automatically use the allocation-free API for Rust 1.27.0+. #103

Closed anp closed 6 years ago

anp commented 6 years ago

Introduces a buildscript to help decide which implementation to use by default. If using a version of Rust >= 1.27.0, what I've renamed the "inline" implementation will be automatically selected. On older versions of Rust, what I'm calling the "heap" implementation (previously the default one) will be selected. Originally I had removed the nightly feature entirely, but then I decided to keep it to preserve semver-major compatibility (let me know if you think this is taking compatibility too far -- it'd be nice to clean the feature up).

Another decision I made was to re-use the rustc_version crate for parsing version numbers rather than rolling our own as @BurntSushi did for regex. This does add build-time dependencies although no extra crates will be added to consumers' binaries. I think it improves clarity slightly and reduces overall churn. Happy to discuss if there's disagreement though.

Test plan: tests pass locally with 1.21.0, 1.27.0, and nightly-2018-06-20. Also will see what CI says.

Closes #102.

anp commented 6 years ago

Looks like nightly builds are failing right now for what I can only assume is an unrelated reason. The 1.21.0 and stable tests are passing on travis.

KodrAus commented 6 years ago

Thanks @anp! I'll dig into this later today.

Yeh it looks like the latest nightly is a bit broken...

KodrAus commented 6 years ago

Thanks @anp! I think we could simplify the cfg attributes a bit if we roll the nightly feature check into the version check in the build script. That way we only set the use_heap_impl if we're on a non-nightly compiler before 1.27.0 and leave nightly as a marker for spin, which only compiles on nightly.

What do you think?

anp commented 6 years ago

@KodrAus I like that idea a lot. Want to take a look at this approach (assuming CI passes after I write this)?

anp commented 6 years ago

I'm not able to reproduce this compiletest failure locally, but it seems like it's probably tripping up on an error message whose format has changed.

anp commented 6 years ago

Rebased and squashed, thanks for the fast turnaround on CI repair!

anp commented 6 years ago

@KodrAus looks like we're all green.

anp commented 6 years ago

Actually, can we hold off on merging for a bit?

Specifically I want to ask about a soundness issue before this becomes part of the de facto 1.0 API for the crate, but it's late here.

eddyb commented 6 years ago

The reasons to hold are both simplification (https://github.com/rust-lang-nursery/lazy-static.rs/issues/102#issuecomment-400959779) and soundness (https://github.com/rust-lang-nursery/lazy-static.rs/issues/105).

anp commented 6 years ago

Alright, thanks for being patient with me here. Unfortunately it's not possible to achieve all of the cleanup @eddyb suggested, as the inline impl requires Drop in statics, which wasn't stable until 1.22.0. If bumping the minimum compiler version of lazy_static to 1.22 is on the table, then we could go from 3 implementations to 2. Since I'm guessing it's not on the table (for good reasons), I've pushed some new commits to try to maximize outcomes for all consumers:

anp commented 6 years ago

Looks like the AppVeyor build failed due to network timeout -- is there an easy way to restart those?

KodrAus commented 6 years ago

Thanks @anp! This looks good to me with a quick scan on mobile. The rejigging of features looks like a good change :+1: I'll dig through again shortly.

Unfortunately there isn't an easy way to retry AppVeyor builds short of updating your branch. Their account model is a bit clumsier for organization ownership so a lot of projects end up tied to individual users.

anp commented 6 years ago

OK, I edited a commit message and re-pushed. CI is re-running.

anp commented 6 years ago

CI passed this time. I think this is good to go, pending review of the extra complexity I've added to the build matrix.

anp commented 6 years ago

Thanks for the feedback, addressed both of these.

anp commented 6 years ago

@KodrAus ping

anp commented 6 years ago

Looks like the crater run came back clean!

KodrAus commented 6 years ago

Merged :tada: We might have a build fail on master again because of #110