-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[SmartHolder] fix(clang-tidy): apply clang-tidy performance fixes #3048
[SmartHolder] fix(clang-tidy): apply clang-tidy performance fixes #3048
Conversation
Just so you're not surprised: the failing Centos 8 build is disabled on this branch. |
aargh - a flake (CI-SH-DEF / 🐍 3 • CentOS7 / PGI 20.9 • x64) Please ignore, or when it's done you can go to Actions and just rerun the CI-SH-DEF workflow. Sadly we cannot rerun just one job. |
Hmm, can't figure out how to rerun. Are the rest flakes? |
Yes, the macos failure is because there was no machine available to run the job :-( |
I initiated the Google-internal global testing for this PR. Results will be available tomorrow morning (Pacific timezone). I ran interactive tests with ASAN and MSAN, covering unit tests in a couple other libraries using pybind11: no issues. |
Good news: The global testing was successful, no issues at all! I'm strongly in favor of merging on master and here in tandem, as soon as possible. @henryiii, what do you think? A couple things I'm confused about:
|
So it's a little more complicated than I initially though. Disabling the checks in the tests folder works, but since there are no targets with performance checks left after that, clang-tidy won't run any performacne checks against the header files (the header files exist as part of the compilation chain from the test folder). I can't easily filter out all the warnings in the tests folder either while leaving them enabled in the include folder. Turning this check on probably warrants its own PR. We will need to enable performance checks in the test folder and then manually nolint all the test cases where we want explicit, but unnecessary copies/moves/etc so that clang-tidy is happy. As such, neither of these PR enables the performance checks to run automatically. (I currently generate these fixes by apply fixes to the whole repo and then resetting everything outside of include). |
Thanks for the explanation! Just to verify for myself that I understand correctly, is the following true?
(I'm totally fine with all of this, if correct.) |
Yeah, here let me edit the config to
Exactly. Also, I just realized the automated fixes didn't fix everything. It didn't fix the 'PYBIND11_ENUM_OP*' macros which copies in the objects there. I can add those fixes by hand to this PR, but I don't want to make you have to rerun it on the Google Wide codebase for performance fix that doesn't seem that consequential. |
Awesome, thanks! Please feel free adding more to this PR. I will have to rerun the testing anyway after the changes are merged here, so that I have less to explain for the internal reviewers. BUT another thing: you probably noticed that we don't have a great track record with fast decision making on the master branch. I'm not sure how soon another maintainer will be able to approve the merge there, but I'm pretty actively working on the smart_holder code and prefer to merge your smart_holder-specific fixes basically straightaway, to avoid a mess of conflicts. I looked through the changes pretty carefully and it looks like the fixes for master are nicely separated from the smart_holder-specific fixes, along file boundaries (I was worried about cast.h and pybind11.h, but the smart_holder code in there seems to be unaffected). Is that also your observation? Basically, the smart_holder-specfic changes are in only two files:
Would it make sense to also merge the .clang-tidy change, to have it active on the smart_holder branch ahead of master (I'm still not sure what the change does)? How about reverting all but these 2 or 3 files here, I'll merge it straightaway, then you only have to worry about the master branch, while I'm pretty sure now that there aren't any complicated merge conflicts in the making. |
@rwgk Done |
For reference, all the .clang-tidy change does is remove a false positive from one of the checks. |
Thanks Aaron! |
Description
Suggested changelog entry: