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

Fix clang-tidy step on CircleCI #3978

Merged
merged 4 commits into from
Jul 18, 2019

Conversation

msimberg
Copy link
Contributor

Any background context you want to provide?

The clang-tidy checks have been disabled (unintentionally) on CircleCI since I changed it to do use a shallow clone. We still need the shallow clone to avoid copying the whole repo between steps, but I've changed it to do a full clone just for the clang-tidy step.

@msimberg msimberg added this to the 1.4.0 milestone Jul 15, 2019
@msimberg msimberg changed the title Clang tidy circleci Fix clang-tidy step on CircleCI Jul 15, 2019
hkaiser
hkaiser previously approved these changes Jul 15, 2019
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@sithhell
Copy link
Member

TBH, I think it would be best to use the CMAKE_CXX_CLANG_TIDY functionality and remove the script entirely. This catches more errors and doesn't need to be maintained...
Just make sure that the .clang-tidy file is being used.

@msimberg
Copy link
Contributor Author

msimberg commented Jul 16, 2019

Good idea. I didn't realize there was such an option to CMake. I'll update it to use that instead.

Since it'll check all files I'll need some time to fix errors that have appeared since it went out of business...

@hkaiser
Copy link
Member

hkaiser commented Jul 16, 2019

Does CMAKE_CXX_CLANG_TIDY work if we only have a shallow copy of our repo? Or will this always run clang-tidy on all targets?

@msimberg
Copy link
Contributor Author

This will run it on all targets, so no need for a full clone. It's potentially a bit slower to do it like this (haven't timed it) but gets rid of the additional script.

hkaiser
hkaiser previously approved these changes Jul 16, 2019
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@@ -444,7 +444,7 @@ namespace hpx { namespace lcos

return all_reduce(
hpx::find_from_basename(std::move(name), root_site),
std::move(local_result), std::forward<F>(op), this_site);
std::forward(local_result), std::forward<F>(op), this_site);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hkaiser could you please check that the move here and in all_to_all.hpp weren't intentional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forward looks like the right thing to use. Thanks for fixing this!

@msimberg msimberg force-pushed the clang-tidy-circleci branch 3 times, most recently from cb6305b to 263727d Compare July 17, 2019 12:49
@msimberg msimberg merged commit 4ab6984 into STEllAR-GROUP:master Jul 18, 2019
@msimberg msimberg deleted the clang-tidy-circleci branch July 18, 2019 09:32
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.

3 participants