Skip to content

Conversation

@rwgk
Copy link
Collaborator

@rwgk rwgk commented Jul 12, 2021

Follow-on to PR #3051 by @Skylion007 .

NOTE: This PR only changes C++ test code. Python test code and production C++ code are completely unaffected.

This PR removes most NOLINT comments, changing the C++ test code instead to resolve the clang-tidy errors.

All changes are of this form:

-T arg
+const T &arg

The only noteworthy special case is a change of the following kind in test_exceptions.cpp, which is to avoid a MSVC (all versions) warning about unreachable code:

-if (...) return true;
-return false;
+bool retval = false;
+if (...) retval = true;
+return retval;

This PR reduces the occurrence of NOLINT in the pybind11 repo from 103 to 22. Most of the rest need to stay, a handful could maybe changed meaningfully but will require corresponding changes in the Python test code (beyond the scope of this PR / best handled in separate PRs).

See also: #3087 (comment)

clang-tidy-diff was applied.

Additional comprehensive Google-internal testing (identical observations for PR applied to master and smart_holder branch):

  • ASAN: clean
  • MSAN: clean
  • UBSAN: clean except two long-standing-already-failing tests (test_eigen, test_numpy_dtypes)
  • TSAN: clean except two long-standing-already-failing tests (test_gil_scoped, test_iostream)

Note: Google-global testing was not run but is pointless in this case, because all changes are in pybind11/tests only.

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 12, 2021

Hi @Skylion007, this PR should be good to go in now I believe.

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 12, 2021

Thanks Aaron and Henry!

@rwgk rwgk merged commit 2d46869 into pybind:master Jul 12, 2021
@rwgk rwgk deleted the nolint_reduction branch July 12, 2021 20:10
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 12, 2021
@rwgk
Copy link
Collaborator Author

rwgk commented Jul 12, 2021

I'm deleting the "needs changelog", I think the clang-tidy work is very well covered already by listing the previous PRs.

@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jul 12, 2021
@henryiii
Copy link
Collaborator

That's correct.

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