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

Improve compile-time if conditions handling #126

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

GuillaumeGomez
Copy link
Contributor

Fixes #115.

It finally allows to write:

{% if x is defined && x == 12 %}

without having compile-time errors because of the x == 12 part. :)

@GuillaumeGomez
Copy link
Contributor Author

Fixed fmt. :)

@Kijewski Kijewski self-requested a review August 9, 2024 19:09
@Kijewski
Copy link
Collaborator

Kijewski commented Aug 9, 2024

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 {
Copy link
Collaborator

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?

Copy link
Contributor Author

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 {
   |

Copy link
Collaborator

@Kijewski Kijewski left a 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! :)

@Kijewski Kijewski merged commit d5710c9 into rinja-rs:master Aug 13, 2024
17 checks passed
@GuillaumeGomez GuillaumeGomez deleted the improve-compile-time-if branch August 13, 2024 09:51
@GuillaumeGomez
Copy link
Contributor Author

I'm not super happy about it either, that's why I added the contains_bool_lit_or_is_defined field to only clone when it's actually needed to limit performance impact.

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.

Improve is (not) defined feature
2 participants