WIP Improved support for visibility restrictions

[fuine]

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

Closes #87

[fuine]

I'm not sure if this is correct, and why previously call to __lazy_static_internal! macro was put in here. This also somewhat duplicated the code. Can someone look at it?

[Kimundi]

I'm not aware of a deeper reason for the duplication. As far as I can remember, when the internal macro split from the public one, this just happend with the idea that the publich one acts as the entry point for the internal one, which then just keeps to itself during expansion.

However, there is no reason for why it couldn't do the mutual recursion back into the public one instead, as done in this PR.

The only thing I can imagine is macro 1.0 weirdness in regard to visibility. As in, depending on how its imported, the internal macro might not see the public one during expansion. Not sure if this can happen here though.

[fuine]

Thanks for the information. I think that you should decide whether or not you like current implementation better than the previous one. If you do then you can merge the PR, otherwise let me know and I'll revert some changes I made and adapt it so that it plays well with all possible visibility restrictions.

[fuine]

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]

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]

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]

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]

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 #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]

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

[fuine]

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

[KodrAus]

Sounds good! Thanks @fuine!

[fuine]

@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]

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]

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