search

brendanzab / gl-rs

An OpenGL function pointer loader for Rust
Apache License 2.0
678 stars 117 forks source link

Replace '_priv: ()' with '#[non_exhaustive]'. #521

Closed feuerste closed 4 years ago

feuerste commented 4 years ago

Since Rust 1.40 non_exhaustive is stable, which is preferred over _priv, see also https://rust-lang.github.io/rust-clippy/master/#manual_non_exhaustive.

This PR adapts everything to the latest recommended way.

Lokathor commented 4 years ago

This raises the minimum rust version for not much actual gain. I don't think it should be used.

feuerste commented 4 years ago

True, it raises the minimum Rust version. At the same time more restrictive CIs using e.g. cargo clippy -- -D warnings currently fail for Rust beta and nightly and soon will fail for stable, too, so I personally consider this quite a gain. See e.g. https://travis-ci.org/github/cartographer-project/point_cloud_viewer/builds/694230893

Lokathor commented 4 years ago

you should not do that in CI exactly because it can break your build when you upgrade stable to stable and new warnings are created.

gl is a very foundational crate, and it has extremely low requirements from the language because it's essentially all FFI declarations and minimal logic of its own. It should preserve support for as many versions of rust as possible over trying to support pedantic warnings about new language features in the latest compiler.

If anything, this should be an update to gl_generator with a new (off by default) flag in the generator.

feuerste commented 4 years ago

Thanks for your explanation. I still want to keep clippy in the CI to ensure that every contributor adheres to good coding style. Anyway, you also have a good point in being as backwards compatible as possible, so I will fix this by adding another #[allow(clippy::manual_non_exhaustive)] around the bindings inclusion. A feature flag IMHO is overkill, so I will just close this PR.

Lokathor commented 4 years ago

There are plans to have cfg eventually be able to detect more, such as the version of rust being used. At that point it would probably make sense to, as a one time cost, do a breaking change to 0.15 (or whatever the next crate version is by then) and move up the minimum rust version to there so that the crate can use the new detection system from then on.

feuerste commented 4 years ago

@Lokathor I just stumbled upon https://github.com/dtolnay/rustversion which seems to be pretty useful in version detection... Please keep non_exhaustive in mind when you do the update :)

Lokathor commented 4 years ago

Ah, to be clear: I don't directly manage this crate, I just monitor the repo and comment because I'm in the gamedev-wg.