Skip to content

NOLINT reduction #3096

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 4 commits into from
Jul 12, 2021
Merged

NOLINT reduction #3096

merged 4 commits into from
Jul 12, 2021

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