Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl TryFrom<T::Type> for BitFlags<T> #14

Merged
merged 5 commits into from
Oct 4, 2019

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Aug 28, 2019

Misc notes:

  • Submitting this for initial feedback on the approach, still needs documentation and tests
  • This raises the minimum rustc version to 1.34.0 (from 1.31.0 currently due to syn 1.0 I think?). If that's too recent we could gate this behind a feature flag instead.
  • This adds a std flag because although TryFrom doesn't have a type Error: std::error::Error bound, it's often useful to have that impl. I'm wary of making it default though, because it still seems ideal to keep the core crate no_std by default?
    • (I'm personally wary of any default Cargo flags in general because it feels way too easy for a dependency to not realise when it should be using default-features = false)
  • Hard-codes the list of numeric types because generic/blanket impls don't mesh well with TryFrom. I was hoping the proc macro impl'ing it for BitFlags<Self> might work but that's not good enough for the orphan rules it seems.
  • BitFlagNum modified to include fmt::Debug constraint because we kind of want #[derive(Debug)] struct S<T>(BitFlags<T>) to work?
    • This is making me think about generic BitFlags<T> in context though... We hide BitFlagsRaw as an internal trait despite the fact that it's required for BitFlags<T> and thus pretty crucial for doing any generic operations over bitflags... I'm questioning whether we shouldn't re-export this publicly at the root of the crate? (if we don't want manual impls and want to still require use of the proc macro, we can still make the trait fns themselves doc(hidden))
    • There are a few points around the ergonomics of BitFlags<T> vs T that I'd like to address I think... Hm.

enumflags/src/formatting.rs Outdated Show resolved Hide resolved
@arcnmx arcnmx force-pushed the fallible branch 3 times, most recently from fe5149c to 1e95218 Compare August 28, 2019 16:46
arcnmx added 2 commits August 28, 2019 09:47
Otherwise, `T: Debug` isn't enough to imply `BitFlags<T>: Debug`, and
this can get in the way of code generated by #[derive(Debug)].
})
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these should be also exposed as a method. A trait would be enough. In fact, I'm considering #[deprecating] from_bits once this lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I disagree with that philosophy in that I find generic conversion traits often unwieldy to use explicitly when you need them, because type inference often needs help via extra type hints/ascriptions when using something(BitFlags::from) - whereas BitFlags::from_bits assists inference and improves readability. I guess I'm not personally a fan of using conversion traits outside of generic arguments when there's a better explicit option (usually that's ::new I guess).

enumflags/src/lib.rs Outdated Show resolved Hide resolved
enumflags/src/formatting.rs Outdated Show resolved Hide resolved
enumflags/src/lib.rs Outdated Show resolved Hide resolved
pub fn invalid_bits(self) -> T::Type {
self.invalid
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a public API, it'd be nice to have a sentence or two of documentation here. You could even write a doctest which tries to convert a number with some invalid flags and then asserts about the results of truncate and invalid_bits. This would kill two birds with one stone, since there aren't really any tests for this functionality right now.

impl_try_from! { $ty }
impl_try_from! { $($tt)* }
};
($ty:ty) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to use 3 rules here. I've pushed a commit onto your branch that shows how this can be done, though ideally it would be squashed into this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, will make sure to get it all get squashed/cleaned up once it's ready.

@meithecatte meithecatte merged commit 8499f0c into meithecatte:master Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants