Make compareTo more robust in semantics.dart #81083
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
test-exempt: code refactor |
| @@ -2353,7 +2353,7 @@ class _BoxEdge implements Comparable<_BoxEdge> { | |||
|
|
|||
| @override | |||
| int compareTo(_BoxEdge other) { | |||
| return (offset - other.offset).sign.toInt(); | |||
There was a problem hiding this comment.
I think we should add an assert just so that it still crashes in debug mode. Otherwise, I feel we won't ever figure out what the issue is.
There was a problem hiding this comment.
line 2333 has the assert that will trigger before this line crashes.
In b/180268307 we end up with double.NaN for _BoxEdge.offset, which is obviously a bug and shouldn't happen. In debug mode we guard against this with asserts. In release mode, those are turned off and the app crashes because
double.NaN.toInt()is illegal.This change makes the code more robust by avoiding the
toInt()call. The bug where _BoxEdge.offset ends up being double.NaN is still present, though and I haven't been able to reproduce it to investigate why it is happening. :(With this change, instead of crashing the traversal order of the SemanticsNodes may be not as expected.