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

Part F of transition to support modernize-use-default-member-init #5815

Merged

Conversation

gnawme
Copy link
Contributor

@gnawme gnawme commented Sep 18, 2023

Part F of #5665 to add the modernize-use-default-member-init check for clang-tidy

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Two general comments:

  • Let's leave the two 3rdparty files in recognition unchanged for now
  • Please don't do any formatting changes (e.g. whitespace changes) in this PR. Let's limit this PR to the clang-tidy changes. Only exception would be formatting changes required by the Formatting Check code formatting CI job, which might for example require a specific formatting if you change something in registration

@gnawme
Copy link
Contributor Author

gnawme commented Oct 2, 2023

  • Please don't do any formatting changes (e.g. whitespace changes) in this PR. Let's limit this PR to the clang-tidy changes.

I ran clang-format to try to forestall any CI complaints, but, as usual, the results were quite unpredictable. I'll have to look at what CI is doing differently (if anything).

@mvieth
Copy link
Member

mvieth commented Oct 2, 2023

The CI calls the formatting script, which applies clang-format to certain modules. We add new modules there when no open pull requests in those modules exist.
If you also revert the formatting changes in hough_3d.h and implicit_shape_model.hpp, I will start a detailed review

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thank you.

@mvieth mvieth merged commit 669652b into PointCloudLibrary:master Oct 21, 2023
13 checks passed
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