Skip to content

Enable and apply clang-tidy readability and misc fixes. #3052

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 3 commits into from
Jun 21, 2021

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Jun 20, 2021

Description

  • Broke off this PR from the beefer performance once. This just applies some automated fixes to the code base. The main one is a redundant function ptr dereference and forgetting to mark a bunch const methods as such. Enables a bunch of other checks that ensure modern code.

There is one readability improvement with const return types I would like to enable but it causes error sadly that need to be fixed.

Suggested changelog entry:

* Enable and apply checks for more misc, readability, and code modernization improvements to the codebase. Remove redundant function ptr dereference, makes methods const, and uses member-initializers when possible to allow for trivial construction.

@@ -75,8 +77,10 @@ struct C {
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wdeprecated"
#endif
int m7(int x) throw() { return x-7; }
int m8(int x) const throw() { return x-8; }
// NOLINTNEXTLINE(modernize-use-noexcept)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check discourages the user of throw, but since these are explicilty deprecated tests, I add nolints to prevent them from being changed.

@Skylion007
Copy link
Collaborator Author

Ping @rwgk @henryiii

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks Aaron!

@rwgk rwgk requested a review from henryiii June 21, 2021 01:34
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

This all looks very good to me! Thanks! Love seeing that someone is finally filling out the clang-tidy checks, see my comment at #2478 (comment) :)

@henryiii henryiii merged commit 3b30b0a into pybind:master Jun 21, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 21, 2021
@henryiii
Copy link
Collaborator

I'm calling this a fix due to the fact that adding const technically is better, as you can only call const methods on a const object.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021
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