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

Prefer empty() over size() when checking container state #3033

Conversation

SunBlack
Copy link
Contributor

Changes are done by run-clang-tidy -header-filter='.*' -checks='-*,readability-container-size-empty' -fix

Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to perform a find-replace to change from empty() to empty (). Because as it is, we are accepting a PR that breaks PCL's coding style with every change it introduced :/

@SergioRAgostinho SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Apr 24, 2019
@SunBlack
Copy link
Contributor Author

Would it be possible to perform a find-replace to change from empty() to empty (). Because as it is, we are accepting a PR that breaks PCL's coding style with every change it introduced :/

Well if I replace all .empty() by .empty (), this will be 389 changes. You really want this in this PR?

Btw: This is the part of the coding style that I really don't like at all. I don't have an idea until now, which benefit we got by a whitespace before a bracket (for me it decreases readability).

@SunBlack SunBlack closed this Apr 24, 2019
@SunBlack
Copy link
Contributor Author

Ups wrong button, didnt' wanted to close this PR xD

@SunBlack SunBlack reopened this Apr 24, 2019
@taketwo
Copy link
Member

taketwo commented Apr 24, 2019

Yes, this whitespace was a notorious choice for the style guide. (Not made by any of us who are currently active in PCL.) I think when we finally adopt automatic code formatting tools we can change it. But we have to live with it, at least for now.

@SunBlack SunBlack force-pushed the readability-container-size-empty branch from 3b06eb0 to 67c1263 Compare April 24, 2019 10:29
@SergioRAgostinho
Copy link
Member

Well if I replace all .empty() by .empty (), this will be 389 changes. You really want this in this PR?

Thank you for the sanity check ^^ Once we get the formatting tool running on CI I'm open to discuss changes to the style guide.

@SergioRAgostinho SergioRAgostinho merged commit b02e666 into PointCloudLibrary:master Apr 25, 2019
@SergioRAgostinho SergioRAgostinho removed the needs: author reply Specify why not closed/merged yet label Apr 25, 2019
@SunBlack SunBlack deleted the readability-container-size-empty branch April 25, 2019 08:58
@taketwo taketwo changed the title Check for empty container via .empty() instead of with .size() Prefer empty() over size() when checking container state Jan 14, 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.

3 participants