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

fix(clang-tidy): Enable clang-tidy else-after-return and redundant void checks #3080

Merged
merged 6 commits into from
Jul 9, 2021

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Jul 7, 2021

Description

  • Clang-Tidy enable a few useful fixes:
  • VirtualCalls (Check that Virtual Calls are not called in the constructor in the destructor ( they can be called if fully qualified, but they cannot be overridden during construction and destruction). This checks makes it very clear. Luckily, this behavior isn't used anywhere in the codebase, but this check will guard against it.
  • Removes redundant void arguments.
  • Most importantly removes all else-after-returns in the code base and prevents new code from introducing it. This is better style and a part of LLVM Style.

Suggested changelog entry:

* Add clang-tidy checks to further guard code style and remove all else after returns for better code readability. Also encourages following llvm-style (no ``else-after-returns``, proper usage of virtual functions in ctors and dtors).

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.

Awesome :-)

@@ -555,9 +563,8 @@ template <template<typename...> class Tuple, typename... Ts> class tuple_caster
if (!src) return none().release();
if (policy == return_value_policy::take_ownership) {
auto h = cast(std::move(*src), policy, parent); delete src; return h;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please also move the delete and return to separate lines while you're at it?
I had to squint at this for a while until I finally noticed the return h;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rwgk Done.

@Skylion007
Copy link
Collaborator Author

Since I can't assign reviewers yet, ping @henryiii

@Skylion007
Copy link
Collaborator Author

ReOpen to retrigger

@Skylion007 Skylion007 closed this Jul 8, 2021
@Skylion007 Skylion007 reopened this Jul 8, 2021
@rwgk
Copy link
Collaborator

rwgk commented Jul 9, 2021

@henryiii in the meantime I ran this through the Google-global testing: no issues.
Considering that you wrote "I like what I saw" and that this is a pure code health PR that's hardening our CI, I'll go ahead merging so we can move forward with other things.

@rwgk rwgk merged commit b5357d1 into pybind:master Jul 9, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 9, 2021
rwgk pushed a commit that referenced this pull request Jul 9, 2021
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 9, 2021
…r the just-merged PR pybind#3080. Also rerunning clang-format-diff.py to pick up a few fixes missed before.
@Skylion007 Skylion007 deleted the clang-tidy-readability-else-after branch July 12, 2021 20:38
@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