Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

Use Rust's #[non_exhaustive] lint for enums #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luojia65
Copy link

@luojia65 luojia65 commented Jun 22, 2020

Rust provides its own #[non_exhaustive] lint after RFC 2008. Use compiler provided guarantee could make better docs, and a better way for pattern matching without breaking backward compability.

To make a proof of concept of usage, users may need to create another crate. Within one crate (a.k.a. the image-core itself) the non_exhaustive only behaves like normal enums. But in other crates if we need to use this enum (e.g. other crate use image-core's enum), there should always be a reserved _ arm on match blocks whenever all arms are already covered.

This change should be zero-cost with the original way. (If I am incorrect, please reply and inform me, thanks :)

@HeroicKatora
Copy link
Member

HeroicKatora commented Jun 22, 2020

@fintelia Have we discussed the MSRV policy for image-core yet? I can't recall it but it seems sensible to use at least the current stable version at release so this should be fine.

@HeroicKatora HeroicKatora changed the title Use Rust's #[non_exahaustive] lint for enums Use Rust's #[non_exhaustive] lint for enums Jun 22, 2020
@luojia65
Copy link
Author

luojia65 commented Jun 22, 2020

Non-exhaustive was stablized in Rust 1.22.0: https://github.com/rust-lang/rust/pull/45394/files#diff-8fe611e80215042c8d89714f4dd6f8baR390 (if I am correct)

@HeroicKatora
Copy link
Member

It was introduced in 1.22, it was stabilized on enums in 1.40.

@HeroicKatora
Copy link
Member

Otherwise we'd be using it in image 😉

@fintelia
Copy link
Contributor

The main concern just comes down to not disrupting users of the main image crate when we make that one depend on image-core

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

Successfully merging this pull request may close these issues.

3 participants