Skip to content

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

Merged
alice-i-cecile merged 4 commits intoDioxusLabs:mainfrom
Sheepyhead:unreachable-code
Jun 20, 2022
Merged

Added unreachable macro to alleged unreachable code rather than using hidden default value#169
alice-i-cecile merged 4 commits intoDioxusLabs:mainfrom
Sheepyhead:unreachable-code

Conversation

@Sheepyhead
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

Should I add that as part of this PR?

Yes please!

@Sheepyhead
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <[email protected]>
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