Uh oh!
There was an error while loading. Please reload this page.
style: clang-tidy - #2478
Conversation
FYI, this is the full list of checks in Clang 10: (click to expand)(or here: https://clang.llvm.org/extra/clang-tidy/checks/list.html) The goal is not to enable them all, as many of them conflict (google vs. llvm, etc), but to just enable the ones or the groups (minus a few) that are useful. Also, the default checks are pretty safe and probably can be enabled. I partially ran them and don't think there were any changes to (at least the headers) in the current form. |
4704ae8 to
00e4bf2CompareI think that this solves the occasional output failure on Windows CI (and I think I've seen it in GooFit on Unix) - the destructor was incorrect, as discovered by the default analyzer checks in clang-tidy. Here's the page on possible solutions, I chose the private member one, though the fully qualified one might work too. Useful page. |
bstaletic
commented
Sep 12, 2020
That does silence the warning, but it shouldn't have any other effect. When executing the destructor, every member function is treated as if it were non-virtual. All in all, this should have exactly the same assembly output and I'd be very surprised if it has any effect on the flake you're referring to. |
The problem is that the correct destructor is guaranteed to be called, but when it is called (due to destruction order), the vtable for the base is loaded, not the child. So (Sync calls virtual functions too, but those are correct in the base. It's the call from the destructor that's a problem. If it's a clear solution, I could do the fully qualified solution instead if you prefer. |
bstaletic
commented
Sep 12, 2020
This is the part that I wasn't aware, but it makes sense. Thanks for the explanation. |
I wasn't aware of it either, which is why I've been trying to solve this bug (in the context of GooFit, anyway) for over two years... The fully qualified solution is a little cleaner/clearer, will change to that. (Edit: I think I miss-read, the fully-qualified method was just a way to silence the warning and clarify you wanted to do that). |
aebf437 to
00e4bf2Comparehenryiii
commented
Sep 12, 2020
(It is possible I'm wrong here, but it's still slightly more correct this way, and we can watch for that bug in the future) |
bstaletic
commented
Sep 12, 2020
A simple check says that it's not as broken as you seem to suggest. Notice that
Can we come up with a case where "some other |
henryiii
commented
Sep 12, 2020
For pybind11, this only fails on msvc (and NVCC for GooFit). I would rather expect the other compilers already handle this properly. |
YannickJadoul
left a comment
There was a problem hiding this comment.
This is great! :-)
I also really like the incremental approach, for these kinds of changes. As far as I'm concerned, we could merge small batches of these in separate PRs, even. That would keep things more easily reviewable.
The only one I'm a bit skeptical about is modernize-use-using. Not sure it that's so problematic. But yes, it's modernizing our code base, so I'm also not opposed.
Personal request: maybe something that flags C-style casts? (cppcoreguidelines-pro-type-cstyle-cast?) Then again, in the context of the Python C API, C-style casts might make sense, ofc, since it is C.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
henryiii
commented
Sep 13, 2020
C-style casts are required in a few places, I believe, but it would be good to identify those and replace the others. I tried this briefly (as part of several others - doing many at once leads to conflicts, though). I know the cast I worked on recently was not allowed to be written as a static_cast, since it was converting something like I didn't add the NULL/0 -> nullptr because it looked odd to have a nullptr in a C api. :) |
Uh oh!
There was an error while loading. Please reload this page.
Can be in a different PR, though. I can put some effort into that, if you want; not per se expecting you to do it, ofc ;-)
Agreed. That's why I'm a bit hesitant indeed. |
henryiii
commented
Sep 13, 2020
Ahh, I think I didn't try reinterpret_cast. |
00e4bf2 to
73b8b43Comparehenryiii
commented
Sep 13, 2020
On that page I linked, "has fix" doesn't seem to be accurate. |
henryiii
commented
Sep 13, 2020
Assuming you run it (the formula is in the PR), try running it a couple of times, it finds a few more fixes the second time. Remember to clean after you run to get it to recompile. And no multithreaded building if you are changing the source with |
Yes, it's here: https://clang.llvm.org/extra/clang-tidy/checks/readability-qualified-auto.html - that recommends (based on the LLVM style guide) that auto be fully qualified. For example:
Edit: Nevermind, seems we hide differences in constness between Python 2.7, 3.5, and 3.8 using unqualified auto. Could be be improved a bit, with explicit ignores and notes on the ones that may or not be const, but for now, leaving that check off. |
98da5c4 to
7d86d3bComparebstaletic
commented
Sep 13, 2020
For the record, C casts are pretty wild. They try, in order:
For more info see https://en.cppreference.com/w/cpp/language/explicit_cast#Explanation |
wjakob
commented
Sep 15, 2020
Oh wow, that is a nasty issue you've found in I have one objection to some of those changes here, and that's the excessive use of "fancy" C++ cast operators. They definitely do have a place in the standard, and that's when working with polymorphic classes, where some casts can lead to undefined behavior, and others will resolve into complex operations that walk through RTTI tables. But for very basic stuff, like casting something to an |
``` /pybind11/include/pybind11/iostream.h:71:9: warning: Call to virtual method 'pythonbuf::sync' during destruction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall] sync(); ^ /pybind11/tests/test_iostream.cpp:72:5: note: Calling '~scoped_ostream_redirect' }); ```
7d86d3b to
24ee662Comparehenryiii
commented
Sep 15, 2020
I've dropped that cast operator commit (was always supposed to be completely non controversial), and rebased. Going in now, then! |
henryiii
commented
Sep 15, 2020
That's really what it looks like to me. Though I will unconvince myself of this instantly if this fails CI again at some point after the merge. ;) |
rwgk
commented
Sep 15, 2020
I only got a chance to look here now: massive thanks @henryiii ! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This adds the ability to run a clang-tidy list of checks in CI, and adds a few checks to get started. Due to the methodology used, only checks that made changes (one commit per check) were added. Only extremely safe checks were added. Eventually, it would be nice to expand further; there are some very useful/powerful clang tidy checks. You can see the groups here. You can do groups of checks, like
modernize-*(that's C++11),readability-*,portability-*,bugprone-*,cppcoreguidelines-*, orperformance-*(and remove ones that you don't support). You can also disable inline for special cases.Checks added:
overrideis always used when overriding.empty()is used to check empty containers instead of size comparisonsusingused instead oftypedef= defaultused for trivial bodies (except where MSVC 2015 requires otherwise)autoused when the typename would be otherwise duplicated on the same line (not everywhere, as that would be worse, not better!)emplaceused instead of push back for containersI've also added instructions for running this locally (where you can perform the fixes automatically).