-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add clang-tidy to CI with an initial set of checks #274
Conversation
Relates #249. 🙂 |
9a221a0
to
f5fde09
Compare
f5fde09
to
00a5965
Compare
auto [con_res, prod_res] = make_spsc_buffer_resource<data_message>(); | ||
// Note: structured bindings with values confuses clang-tidy's leak checker. | ||
auto resources = make_spsc_buffer_resource<data_message>(); | ||
auto& [con_res, prod_res] = resources; |
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.
The previous version wasn't a reference. Was making it one intentional? Does the same need to be done in the following change?
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.
Concerning the old code:
auto [con_res, prod_res] = make_spsc_buffer_resource<data_message>();
This deconstructs the temporary object into two values. The compiler should break this down into something like this:
auto tmp = make_spsc_buffer_resource<data_message>();
auto con_res = std::move(tmp.first);
auto prod_res = std::move(tmp.second);
For some reason, clang-tidy
gets confused with object lifetimes for the code expanded from the structured binding and complains about a leak that doesn't happen. The new code simply makes the temporary variable visible and skips the moves by simply pointing back into the (now named) temporary object. The new version should expand to something like this:
auto resources = make_spsc_buffer_resource<data_message>();
auto& con_res = resources.first;
auto& prod_res = resources.second;
From a coding perspective, there is no functional change here - other than the temporary object got put on the stack and has a name now. I'd prefer the old code (simply to have a one line less of code) if it wouldn't confuse clang-tidy
.
So yes, having references now is intentional to avoid extra copies. We simply need to place the result of make_spsc_buffer_resource
on the stack for "un-confusing" clang-tidy
regarding the object lifetimes.
auto [rd_2, wr_2] = caf::async::make_spsc_buffer_resource<node_message>(); | ||
// Note: structured bindings with values confuses clang-tidy's leak checker. | ||
auto resources1 = caf::async::make_spsc_buffer_resource<node_message>(); | ||
auto& [rd_1, wr_1] = resources1; |
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.
Same question re: references. The first two changes here make the variables references, but the third one doesn't.
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.
Good catch, the third one is an oversight. I'll fix it.
This PR adds
clang-tidy
to our CI pipeline withbugprone-*
andclang-analyzer-*
checks (plus fixes for the findings) as initial set of checks. To make the PRs easier to review, I leave it at those first two categories and enable more checks in future PRs (already working on enablingperformance-*
checks).I've also added a CMake option (with
configure
switch) to haveclang-tidy
run during development (slows down the build quite a bit).