flex icon indicating copy to clipboard operation
flex copied to clipboard

Does the `while` loop in step 8 ever run?

Open rigdern opened this issue 7 years ago • 1 comments

While reading the code, I noticed that shouldContinue is initialized to false and never gets set to true. I didn't do any verification beyond code inspection.

https://github.com/jordwalke/flex/blob/fd0257ec12617f5d8c12752ce0ec690f15d249fb/src/lib/Layout.re#L1438-L1458

rigdern avatar Jan 28 '18 03:01 rigdern

Great find! It should be initialized to true, I believe. Here's the corresponding portion of that code in C: https://github.com/facebook/yoga/blob/01c2ac3369b6e403e5f7a24b91a01b7bb792eed7/CSSLayout/CSSLayout.c#L1950

We should probably have a test case to catch it.

We have a while loop with "shouldContinue" to emulate a break; since Reason doesn't have break. Normally you would not write idiomatic Reason code in this manner, but since we are transcribing from C, we wanted to make sure the algorithm was as close as possible. It would be fun to clean this up over time to turn it into a more idiomatic style with fewer mutations.

jordwalke avatar Jan 28 '18 06:01 jordwalke