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

Added unreachable macro to alleged unreachable code rather than using hidden default value #169

Merged
merged 4 commits into from
Jun 20, 2022

Conversation

Sheepyhead
Copy link
Contributor

@Sheepyhead Sheepyhead commented Jun 16, 2022

Objective

Fixes #168

Context

This doesn't seem to panic any tests, but this still confers a risk to users that this code has actually been reached by some edge cases, where it would simply fail silently before.

Feedback wanted

  • If you have code depending on taffy, I'd really like to know if you get a panic from this PR, the more real dirty user code we test this on the better

@alice-i-cecile alice-i-cecile added the code quality Make the code cleaner or prettier. label Jun 16, 2022
@mockersf
Copy link
Contributor

mockersf commented Jun 16, 2022

  • If you have code depending on taffy, I'd really like to know if you get a panic from this PR, the more real dirty user code we test this on the better

No need to worry, it's impossible to fall in that case... as long as align_self() doesn't change

taffy/src/style.rs

Lines 475 to 487 in e75a55f

pub(crate) fn align_self(&self, parent: &FlexboxLayout) -> AlignSelf {
if self.align_self == AlignSelf::Auto {
match parent.align_items {
AlignItems::FlexStart => AlignSelf::FlexStart,
AlignItems::FlexEnd => AlignSelf::FlexEnd,
AlignItems::Center => AlignSelf::Center,
AlignItems::Baseline => AlignSelf::Baseline,
AlignItems::Stretch => AlignSelf::Stretch,
}
} else {
self.align_self
}
}

It could be interesting to add a comment on that method to say it should never return AlignSelf::Auto as that's an invariant expected by other part of the code

@TimJentzsch
Copy link
Collaborator

This doesn't seem to panic any tests, but this still confers a risk to users that this code has actually been reached by some edge cases, where it would simply fail silently before.

IMO this is good, it's usually better to fail early and detect these kind of bugs instead of failing silently and causing problems in other areas that are hard to track back.

@Sheepyhead
Copy link
Contributor Author

IMO this is good, it's usually better to fail early and detect these kind of bugs instead of failing silently and causing problems in other areas that are hard to track back.

I agree, still it's something to note about this innocent-looking two-line PR ;)

@Sheepyhead
Copy link
Contributor Author

  • If you have code depending on taffy, I'd really like to know if you get a panic from this PR, the more real dirty user code we test this on the better

No need to worry, it's impossible to fall in that case... as long as align_self() doesn't change

taffy/src/style.rs

Lines 475 to 487 in e75a55f

pub(crate) fn align_self(&self, parent: &FlexboxLayout) -> AlignSelf {
if self.align_self == AlignSelf::Auto {
match parent.align_items {
AlignItems::FlexStart => AlignSelf::FlexStart,
AlignItems::FlexEnd => AlignSelf::FlexEnd,
AlignItems::Center => AlignSelf::Center,
AlignItems::Baseline => AlignSelf::Baseline,
AlignItems::Stretch => AlignSelf::Stretch,
}
} else {
self.align_self
}
}

It could be interesting to add a comment on that method to say it should never return AlignSelf::Auto as that's an invariant expected by other part of the code

Should I add that as part of this PR?

@alice-i-cecile
Copy link
Collaborator

Should I add that as part of this PR?

Yes please!

@Sheepyhead
Copy link
Contributor Author

Should I add that as part of this PR?

Yes please!

Idk if the way I did it works, but it was the best I could think of

Copy link
Collaborator

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

Looks good :)

@alice-i-cecile alice-i-cecile enabled auto-merge (squash) June 16, 2022 20:10
@Weibye
Copy link
Collaborator

Weibye commented Jun 18, 2022

@Sheepyhead Please run cargo fmt on the code then push the changes :) Everything else is good to go 🚀

auto-merge was automatically disabled June 20, 2022 14:44

Head branch was pushed to by a user without write access

@alice-i-cecile alice-i-cecile enabled auto-merge (squash) June 20, 2022 18:41
@alice-i-cecile alice-i-cecile merged commit f52f652 into DioxusLabs:main Jun 20, 2022
@Sheepyhead Sheepyhead deleted the unreachable-code branch June 20, 2022 19:24
jkelleyrtp pushed a commit that referenced this pull request Oct 10, 2022
… hidden default value (#169)

* Added unreachable macro to alleged unreachable code rather than using hidden default value

* Added comment warning

* Fixed formatting

Co-authored-by: Troels Peter Jessen <trojes@tuta.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Make the code cleaner or prettier.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark unreachable code in perform_absolute_layout_on_absolute_children as such
5 participants