search

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

WIP Improved support for visibility restrictions #88

Closed fuine closed 6 years ago

fuine commented 6 years ago

I marked the PR WIP because I'm not 100% sure one change I introduced is correct.

Closes #87

fuine commented 6 years ago

Also, is there a way to check for compiler errors in tests? I have added two cases which should result in compilation errors and I have manually confirmed that they do indeed yield those errors, but I'd rather have it automated.

fuine commented 6 years ago

It looks like Windows build failed due to the wrong version of stable compiler:

Downloading rust-1.16.0-i686-pc-windows-gnu.exe (74,903,600 bytes)...100%

Visibility restrictions have been introduced in 1.18

KodrAus commented 6 years ago

Thanks @fuine! Sorry I don't think I can review this personally.

It looks like there's a RUST_VERSION environment variable in AppVeyor that's controlling this. We'll just need to get that bumped up to 1.18.

For the compile-fail tests, there is the compiletest crate, but I haven't used it personally and looks like it would mean restructuring the crate's tests somewhat to support. Maybe out-of-scope for this particular change.

@Kimundi what do you think?

fuine commented 6 years ago

Thanks, I'm willing to implement tests using compiletest if maintainers think this is a good thing to be introduced to the test suite, probably in a separate PR.

@KodrAus rustc version seems to be pulled from a hardcoded url, which (I just checked) indeed returns version 1.16. After checking appveyor setup proposed by the Rust team it looks like it has changed, and it no longer uses that particular URL. IMO it got deprecated but for some reason the URL persists serving the file (probably legacy reasons).

Kimundi commented 6 years ago

I'd love to have compile tests in this lib! I never got around to adding them myself - would be nice for checking the unused variable warning in the tests as well, and also https://github.com/rust-lang-nursery/lazy-static.rs/issues/73.

If something seems wrong with the appveyor setup feel free to open a PR for fixing it as well - I'm not to familiar with it myself.

KodrAus commented 6 years ago

@fuine I've merged in your fix for the AppVeyor build, would you like to rebase this?

fuine commented 6 years ago

Sure, but I want to land the compiletest support prior to this being merged, I'll open a new PR today/tomorrow

KodrAus commented 6 years ago

Sounds good! Thanks @fuine!

fuine commented 6 years ago

@KodrAus @Kimundi in the meantime, could you take a look at introduced changes? The only thing that I will change after #90 lands is addition of two compiler tests, but modifications of crate source files will stay the same.

fuine commented 6 years ago

Once tests pass I think that it's good to go, but I still would like someone knowledgable to take a look at the change I commented under in the review - maybe there was some subtlety to the way previous code was designed which I didn't spot and my change breaks some corner cases

Kimundi commented 6 years ago

Alright, looks good! Thanks for the contribution. :)