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

Does the while loop in step 8 ever run? #15

Open
rigdern opened this issue Jan 28, 2018 · 1 comment
Open

Does the while loop in step 8 ever run? #15

rigdern opened this issue Jan 28, 2018 · 1 comment

Comments

@rigdern
Copy link

rigdern commented Jan 28, 2018

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.

flex/src/lib/Layout.re

Lines 1438 to 1458 in fd0257e

let shouldContinue = {contents: false};
while (j.contents < childCount && shouldContinue.contents) {
child.contents = node.children[j.contents];
if (child.contents.style.positionType === Relative) {
if (child.contents.lineIndex !== i) {
shouldContinue.contents = false
} else if (isLayoutDimDefined(child.contents, crossAxis)) {
lineHeight.contents =
fmaxf(
lineHeight.contents,
layoutMeasuredDimensionForAxis(child.contents, crossAxis)
+. getMarginAxis(child.contents, crossAxis)
);
j.contents = j.contents + 1
} else {
j.contents = j.contents + 1
}
} else {
j.contents = j.contents + 1
}
};

@jordwalke
Copy link
Owner

jordwalke commented Jan 28, 2018

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.

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

No branches or pull requests

2 participants