Skip to content
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

Merged
merged 2 commits into from
Sep 13, 2022

Conversation

Neverlord
Copy link
Member

This PR adds clang-tidy to our CI pipeline with bugprone-* and clang-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 enabling performance-* checks).

I've also added a CMake option (with configure switch) to have clang-tidy run during development (slows down the build quite a bit).

@Neverlord
Copy link
Member Author

Relates #249. 🙂

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;
Copy link
Member

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?

Copy link
Member Author

@Neverlord Neverlord Sep 6, 2022

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;
Copy link
Member

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.

Copy link
Member Author

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.

@timwoj timwoj merged commit 1ddc6f7 into master Sep 13, 2022
@timwoj timwoj deleted the topic/neverlord/clang-tidy branch September 13, 2022 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants