-
Notifications
You must be signed in to change notification settings - Fork 438
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
[Code Health] clang-tidy cleanup, part 1 #2990
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2990 +/- ##
==========================================
+ Coverage 87.12% 87.67% +0.55%
==========================================
Files 200 190 -10
Lines 6109 5853 -256
==========================================
- Hits 5322 5131 -191
+ Misses 787 722 -65
|
@msiddhu Can you update the description with the rationale behind these changes? Seems some of the changes in the API could result in ABI incompatibility. |
25e859b
to
cd1f9fc
Compare
Thanks for the cleanup. Reopening and setting as draft, we definitively don't want this work to be forgotten ;-) |
cd5161e
to
3202581
Compare
@msiddhu Thanks for working on this, this cleanup is much needed. From experience in similar cleanup for warnings: the complexity in resolving the cleanup is not so much individual changes, but making sure the overall task makes progress, including with reviews and merges. Several activities are needed:
The clang-tidy cleanup will be an iterative process, the best is to do it by small size chunks, instead in one massive PR. Please proceed as follows:
From a quick look at the PR current content, the review should be fast for this, so I think we can aim to merge this chunk soon. Please resolve the merge conflicts and flag the PR as ready to review. Again, thanks for taking this up. |
Not sure why CI is stuck. First thing is to resolve conflicts anyway, the next commit should restart CI. |
warning via auto FIX-IT and some manually. Supressed some warnings where ever needed (mosty in test cases)
@marcalff Thanks for your comments.
Question: Should the CI for clang-tidy be provided in this PR or new PR for that?Still one check is failing (Bazel noexcept). I'll resolve that soon and make the PR ready to review. Please let me know if anything is needed. Thanks. |
@marcalff Marking it ready to review. |
Was there any need to add try-catch for OTELResourceDetector::Detect()? The current CI is strict enough not to allow adding exception in the code - for both API and SDK. PS - Would be good to discuss in the community meeting if the enforcement should be only for the API (which is required to ensure exceptions don't cross the ABI boundary). |
Yes, There is a possibility to get exception in OTELResourceDetector::Detect(). I understood the CI. So, I have added exceptions currently to following:
Please let me know the decision. Happy to make changes. Thanks. |
I think both are correct, the static analysis just goes a step further: /tmp$ echo "#include <stdexcept>
void bar() {
throw std::runtime_error(\"An error occurred\");
}
void foo() noexcept {
bar();
}
int main() {
foo();
return 0;
}" > ./test_exception.cpp
$
$ g++ -Wall -Wextra -Werror -std=c++17 test_exception.cpp -o example
$ clang-tidy test_exception.cpp -checks='-*,bugprone-exception-escape' -- -std=c++17
2 warnings generated.
/tmp/test_exception.cpp:7:6: warning: an exception may be thrown in function 'foo' which should not throw exceptions [bugprone-exception-escape]
void foo() noexcept {
^
/tmp/test_exception.cpp:11:5: warning: an exception may be thrown in function 'main' which should not throw exceptions [bugprone-exception-escape]
int main() {
^
$ |
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.
Thanks for the cleanup.
This patch contains a lot of very good fixes, and also will improve performances with a lot of object copy that are avoided.
See some changes around CURL, it really needs to use the long type.
About try/catch blocks with exceptions, a bigger discussion is needed with maintainers to see how best to proceed, but given the topic, all the exception related work should probably be done separately.
Yes, I feel about I'll remove the changes related to Thanks @marcalff for taking time to review the long PR. Now I understood what I have done wrong. I'll update PR and have comments some comments soon. About the previous comments:
|
…tion handling which was present previously. Reverted by the increment function in circular_buffer.cc.
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 for the cleanup.
Waiting for:
and will merge after that. |
Contributes to #2053
Changes
Please provide a brief description of the changes here.
Added the
.clang-tidy
configuration file. Solved some warnings using auto FIX-IT and some manually. Suppressed some warnings where needed (mostly in test cases, headers, and recursive functions).Started with configurations that are a good starting point, inspired by Google Cloud clang-tidy and AWS Palace clang-tidy.
More checks could be enabled in further cleanups.
Points to note:
.append
instead of+
,'\n'
instead ofstd::endl
, added missed comma'susing namespace
which are not used in source code.long
toint64
Google Coding conventionsCleanup in following Areas:
Please review the differences in indented block as they'll be shown as complete block making it difficult to find differences. Hopefully this cleanup may provide some performance improvements.
Remaining Work:
More cleanup, Integrate in CI .
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes