-
Notifications
You must be signed in to change notification settings - Fork 17
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
Improve compile-time if conditions handling #126
Improve compile-time if conditions handling #126
Conversation
6599ed2
to
411f2b7
Compare
Fixed fmt. :) |
Will review in the next few days, today I'm wiped. :) |
@@ -7,7 +7,7 @@ pub(crate) trait Splitter: Copy { | |||
fn split<'a>(&self, haystack: &'a str) -> Option<(&'a str, &'a str)>; | |||
} | |||
|
|||
impl<T: Splitter + ?Sized> Splitter for &T { | |||
impl<T: Splitter> Splitter for &T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, why didn't clippy like that? Because Splitter is not pub
, so it's known that there won't be any unsized Splitters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the output:
warning: `?Sized` bound is ignored because of a `Sized` requirement
--> rinja_parser/src/memchr_splitter.rs:10:20
|
10 | impl<T: Splitter + ?Sized> Splitter for &T {
| ^^^^^^
|
note: `T` cannot be unsized because of the bound
--> rinja_parser/src/memchr_splitter.rs:10:9
|
10 | impl<T: Splitter + ?Sized> Splitter for &T {
| ^^^^^^^^
= note: ...because `Splitter` has the bound `Copy`
= note: ...because `Copy` has the bound `Clone`
= note: ...because `Clone` has the bound `Sized`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_maybe_sized
= note: `#[warn(clippy::needless_maybe_sized)]` on by default
help: change the bounds that require `Sized`, or remove the `?Sized` bound
|
10 - impl<T: Splitter + ?Sized> Splitter for &T {
10 + impl<T: Splitter> Splitter for &T {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super happy that the Expr
need to get cloned, but I don't see a better implementation. Adding e.g. an enum { &Expr | Box<Expr> }
type would make the code unreadable, because the change would be viral: any &Expr
would need to be changed down the line.
And since the conditions should not be excessively big (ensured by our nesting level check), this should be fine.
Thanks for the PR! :)
I'm not super happy about it either, that's why I added the |
Fixes #115.
It finally allows to write:
without having compile-time errors because of the
x == 12
part. :)