-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Use in-class initializers, default constructors, class -> struct #4842
Conversation
Do we even support this properly in our checks? I remember a mention in a ticket that class initializers are currently not handled at all. We should compare the AST/debug output. I also have the feeling this should have been done at a point where we don't have like 30 PRs ready to review/merge since this will probably cause several conflicts. Also we should keep an eye on the performance to make sure some implicit move constructors and move assignment operators do not disappear. #4836 for instance fixed cases where they were missing to begin with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please enable the currently disabled modernize-use-default-member-init
and readability-redundant-member-init
checks in clang-tidy and also adjust clang-tidy.md
. If we are doing this we should make it complete and avoid regressions in future commits.
bool analyzeTerminate; | ||
Analyzer::Action actions{ Analyzer::Action::None }; | ||
bool analyzeOnly{}; | ||
bool analyzeTerminate{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be initialized as = false
. Using {}
will not always zero initialize the memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? Do you mean the bool
members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But bool b{};
will always be false
.
Result(Type type, const Token *token) : type(type), token(token) {} | ||
const Token *token; | ||
const Token* token{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use = nullptr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Seems more verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But its more explicit to what it would be initialized to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is our style, then we should enforce it with some kind of check. Otherwise it's just a preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For information.. I feel more comfortable with = nullptr
also but I have no strong opinion.
If something weird/awkward/etc. comes from the clang-tidy results just suppress it and point it out to me so I can have a look and maybe report it upstream. There's a reason some of those have been disabled for so long. |
@@ -1,5 +1,70 @@ | |||
--- | |||
Checks: '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,google-explicit-constructor,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-redundant-member-init,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics,-misc-use-anonymous-namespace,cert-err34-c' | |||
Checks: > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This format is not supported in older clang-tidy versions which we implicitly still support as we also support older Clang versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who uses our .clang-tidy
except us? There is a way to get line-wise formatting in older versions as well, but it's ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who uses our
.clang-tidy
except us?
Every user who has an editor which support clang-tidy.
There is a way to get line-wise formatting in older versions as well, but it's ugly.
I know. If possible I would have used that already. The problem is that YAML parser in LLVM is not complete and only supports the bare minimum of features required. So, no.
The multi-line format was not added until about 3 years ago IIRC. We need to do some tests to figure out which version that was. As ubuntu 20.04 is definitely one of the operating systems we should properly support as a dev system it is quite possible we slightly missed that.
Another problem is, that it fails silently. So it will just ignore the option and will use the default checks.
They also just recently changed the default format to more modern YAML which is also not backwards compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks: '
,*,
,-bugprone-assignment-in-if-condition,
,-bugprone-easily-swappable-parameters,
'
This is what I meant above, it should work with older clangs.
But even if there were developers (not users) on older systems, using an old clang makes little sense, since we have the current version in our CI, which needs to pass eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I meant above, it should work with older clangs.
I will give it a spin later today. I have several older versions installed.
But even if there were developers (not users) on older systems, using an old clang makes little sense, since we have the current version in our CI, which needs to pass eventually.
I think using any LTS version is justified. ubuntu 20.04 (still supported until at least 2025) comes with Clang 10 out of the box and it seems there's also an official repo with Clang 13. But distros rarely update such major packages after release. There's also the possibility to install the latest via the LLVM repo. But it also depends on your IDE integration. But the length of the LTS support windows have been getting quite ridiculous. I happy that non-LTS versions usually get discarded as soon as the next non-LTS ones are out. But unfortunately that solidifies the usage of older LTS versions.
Just take a look at the LLVM bug tracker. They frequently get reports for older versions.
Or ubuntu Touch - they have been working for years now to move off ubuntu 16.04 and just recently achieved their move to ubuntu 20.04. It is unlikely that their CI is using the latest compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need to open up the discussions here so we don't go off-topic so often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need to open up the discussions here so we don't go off-topic so often.
any reason to not use the forum on sourceforge? We don't need several forums.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all development happens on Github, we might also close Sourceforge and focus on a single site. Probably more people are already on Github than on Sourceforge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every user who has an editor which support clang-tidy.
That sounds a lot. But cppcheck users will not see this file. This is not installed.
I would say it's only cppcheck developers that might use this. And well most developers will likely not use this file neither locally. I assume it makes sense to use it locally only if you have the same clang-tidy version as CI because there are different checkers in different versions.
@@ -5998,7 +5987,7 @@ static void insertNegateKnown(std::list<ValueFlow::Value>& values, const std::li | |||
|
|||
struct ConditionHandler { | |||
struct Condition { | |||
const Token *vartok; | |||
const Token* vartok{}; | |||
std::list<ValueFlow::Value> true_values; | |||
std::list<ValueFlow::Value> false_values; | |||
bool inverted = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why explicit false
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK we have not yet decided on a mandatory convention for initializers.
Something I mentioned before. We should check that this kind of code is actually (properly) supported by us. Would be bad if this would cause us to miss issues in the selfcheck from now on. That should prevent this from being merged though. I would also like to see an approval from @pfultz2 . |
The only issue that I'm aware of is that side effects in initializers are not considered (https://trac.cppcheck.net/ticket/11205). But we don't have in this PR. |
Great! I was afraid that initializations with POD types like void f() {
bool b1{};
if (b1) {} // no warning
bool b2 = false;
if (b2) {}
void* p1{};
if (p1) {} // no warning
void* p2 = nullptr;
if (p2) {}
} |
So, how about this? |
There is a conflict now. But if you solve that feel free to merge. I find it hard to determine that there is no changed behavior anywhere though. But overall I am fine with this refactoring 👍 |
Concerns about .clang-tidy have been addressed.
We should check how this affects our AST to maybe find some shortcomings. I am also a bit creeped out that you enums with invalid values this way. I haven't looked into that yet. |
No description provided.