Skip to content

Invert if statement in filesystem for clearer intent. #31865

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

Merged
merged 1 commit into from May 19, 2020
Merged

Invert if statement in filesystem for clearer intent. #31865

merged 1 commit into from May 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 18, 2020

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
From the top down of the code, the code checks to see if the file sizes are different, then to check if they are 0, and then to compare the memory contents. However, the first early return in that portion is to return DifferentContents, and then check for 0 sizes and if they are then return SameContents. However on the bottom, it checks to see if the memory difference is 0.

To make the code clearer in its intent, I changed the if statement so it would return DifferentContents if memcmp was non-0, and then otherwise say they are the same contents.

@CodaFi
Copy link
Contributor

CodaFi commented May 18, 2020

While you're here, could you explicitly brace all of the blocks in swift::areFilesDifferent? It's a pet peeve of mine to see multi-line blocks without them.

@ghost
Copy link
Author

ghost commented May 18, 2020

@CodaFi done!

@CodaFi
Copy link
Contributor

CodaFi commented May 18, 2020

@swift-ci smoke test

@CodaFi
Copy link
Contributor

CodaFi commented May 18, 2020

@swift-ci smoke test Linux

@CodaFi
Copy link
Contributor

CodaFi commented May 19, 2020

@CodaFi CodaFi merged commit cba3c23 into swiftlang:master May 19, 2020
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

Successfully merging this pull request may close these issues.

2 participants