-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
clang/tidy: Fixes (the rest) #29660
clang/tidy: Fixes (the rest) #29660
Conversation
cc @wbpcode this gets the codebase (except mobile and a broken tool) passing clang-tidy im not sure about some of the fixes so it will need to be checked carefully anything i was particularly unsure about, i used a some of these may be flagging important things so these probably need to be checked even more carefully i think |
f0e2219
to
e5a8d21
Compare
687d71a
to
1c65bed
Compare
separating out PRs for source/contrib/test |
b1119d9
to
5e002d7
Compare
2a429c6
to
3b8bc4f
Compare
/retest (testing) |
/retest testing |
1 similar comment
/retest testing |
bbc4b76
to
20f1afd
Compare
@wbpcode this contains the remaining fixes/etc - a lot of with the help of @ravenblackx ive managed to improve some of the fixes, but it would be good to remove more nolints - happy to iterate this before we land to get fixes right |
20f1afd
to
022022e
Compare
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.
LGTM overall. only one question.
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.
LGTM. Thanks. We add lots of NOLINT to the code base. But I think it is ok. The main responsibility of this PR is fix the CI rather than to improve the code quality. The last one could be achieved gradually by all contributors' work.
@@ -115,7 +116,7 @@ template <class T> class RefcountPtr { | |||
// forth. | |||
#if __cplusplus < 202002L | |||
template <class T> static bool operator==(std::nullptr_t, const RefcountPtr<T>& a) { | |||
return a == nullptr; | |||
return a == nullptr; // NOLINT(clang-analyzer-cplusplus.Move) |
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.
weird, why this line need this nolint...
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.
no idea tbh - but confirmed its necessary
./envoy/stats/refcount_ptr.h:119:10: error: Method called on moved-from object 'rp4' [clang-analyzer-cplusplus.Move,-warnings-as-errors]
return a == 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.
also don't know why, orz.
re to the coverage failure, seems the new If you think it's OK, I could try to create a patch to your branch to update the code and avoid the |
that would be amazing |
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com> Signed-off-by: Ryan Northey <ryan@synca.io>
3cb1747
to
138e970
Compare
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.
LGTM. Thanks.
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]