Skip to content

maint(clang-tidy): Bugprone enable checks - #3166

Merged
Skylion007 merged 22 commits into
pybind:masterfrom
Skylion007:bugprone-enable-checks
Aug 6, 2021
Merged

maint(clang-tidy): Bugprone enable checks#3166
Skylion007 merged 22 commits into
pybind:masterfrom
Skylion007:bugprone-enable-checks

Conversation

@Skylion007

@Skylion007Skylion007 commented Aug 2, 2021

Copy link
Copy Markdown
Collaborator

Description

  • Enables additional clang-tidy checks for bugprone-behavior.

Suggested changelog entry:

* Enable most bugprone checks in clang-tidy and fix the found potential bugs and poor coding styles.

@Skylion007
Skylion007 requested a review from rwgkAugust 2, 2021 17:00
Comment thread.clang-tidy
Comment threadinclude/pybind11/numpy.h Outdated
@rwgk

rwgk commented Aug 3, 2021

Copy link
Copy Markdown
Collaborator

I just took a super quick look after seeing the emails about your last two commits. Does that mean enable_if isn't needed in this case, just deleting the copy and move constructors does the trick?

@Skylion007

Skylion007 commented Aug 3, 2021

Copy link
Copy Markdown
CollaboratorAuthor

I just took a super quick look after seeing the emails about your last two commits. Does that mean enable_if isn't needed in this case, just deleting the copy and move constructors does the trick?

Unfortunately, the default copy and move destructors are actually used so I do need to do the enableif trick. It passes clang-tidy but fails compilation.

@Skylion007
Skylion007 marked this pull request as ready for review August 4, 2021 16:14
Comment threadinclude/pybind11/embed.h Outdated
Comment threadinclude/pybind11/detail/common.h Outdated
@Skylion007

Copy link
Copy Markdown
CollaboratorAuthor

@henryiii Ping. Would appreciated your feedback on this PR. :)

@rwgk

rwgk commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

ICC flaked, I don't remember seeing that exact flake before, but it's download related, it doesn't even get to the configure stage. Maybe just try again?
(I have one suggestion though (I think). Hang on.)

rwgk
rwgk approved these changes Aug 5, 2021
Comment thread.clang-tidy
Comment threadinclude/pybind11/detail/common.h Outdated
Comment threadinclude/pybind11/detail/type_caster_base.h Outdated
Comment threadinclude/pybind11/gil.h Outdated
Comment threadinclude/pybind11/numpy.h
Comment threadtests/test_class.cpp Outdated
@henryiii

Copy link
Copy Markdown
Collaborator

Looks good, would second @rwgk's suggestions; a couple of places need comments as to why they are doing what they are doing and bugprone-exception-escape is probably less confusing if handled in a different PR (though personally not highly against adding a nolint for it if you know it won't pass, even if it's not enabled, but fine to wait and I do see that seeing a no lint for something that is not being linted is likely quite confusing).

@Skylion007

Copy link
Copy Markdown
CollaboratorAuthor

@henryiii Need your approval to merge. :)

rwgk
rwgk approved these changes Aug 6, 2021

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks Aaron!

@rwgk

rwgk commented Aug 6, 2021

Copy link
Copy Markdown
Collaborator

Looks like you lost a comma in numpy.h.

@rwgk

rwgk commented Aug 6, 2021

Copy link
Copy Markdown
Collaborator

@henryiii Need your approval to merge. :)

We are already two maintainers.
This is pure code health.
I don't want to by-pass anyone, but to me this clearly is fine with just one approval.
Note that even in a shop like Google, where you could in theory bring down a service for billions, this is enough approvals coverage. Here we're just working on master codehealth.
Caveat: at Google we cannot submit with a broken CI. But I trust nobody would do that here.

@Skylion007
Skylion007 merged commit 3893f37 into pybind:masterAug 6, 2021
@Skylion007
Skylion007 deleted the bugprone-enable-checks branch August 6, 2021 18:30
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Aug 6, 2021
rwgk added a commit to rwgk/pybind11 that referenced this pull request Aug 14, 2021
@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021
@rwgkrwgk mentioned this pull request Feb 10, 2023
Sign up for freeto 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

@Skylion007@rwgk@henryiii