clang-tidy: Enable more checks #149

psalz · 2022-09-27T09:37:47Z

So far we've used clang-tidy only to enforce naming conventions. This PR enables a whole set of additional checks as suggested in #127.

Notably this includes misc-const-correctness, a check that was recently added in clang-tidy 15. While we don't yet use clang-tidy 15 in CI, this patch includes a (semi-automatic) fixup pass that was done for all const-correctness violations found through that check. Unfortunately the check seems to be a little buggy still (producing false positives in certain situations) which is why I've decided to not turn it into an error for now.

Here's a summary of the remaining clang-tidy warnings we produce with the set of checks enabled in this patch:

Found 465 diagnostics (out of 2880 parsed; 2415 were duplicates).
   96: readability-qualified-auto
   51: readability-function-cognitive-complexity
   46: bugprone-unchecked-optional-access
   33: cppcoreguidelines-special-member-functions
   21: cppcoreguidelines-init-variables
   18: readability-implicit-bool-conversion
   16: bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions
   16: readability-named-parameter
   15: performance-unnecessary-value-param
   14: cppcoreguidelines-avoid-non-const-global-variables
   13: cppcoreguidelines-pro-bounds-constant-array-index
   12: clang-analyzer-deadcode.DeadStores
   12: cppcoreguidelines-pro-bounds-array-to-pointer-decay
   11: readability-isolate-declaration
   10: cppcoreguidelines-pro-type-member-init
    9: cppcoreguidelines-pro-type-static-cast-downcast
    8: bugprone-exception-escape
    6: cppcoreguidelines-pro-type-vararg
    6: readability-else-after-return
    6: cppcoreguidelines-non-private-member-variables-in-classes
    5: cppcoreguidelines-pro-type-reinterpret-cast
    4: readability-redundant-access-specifiers
    3: performance-faster-string-find
    3: cppcoreguidelines-pro-type-const-cast
    3: cppcoreguidelines-owning-memory
    3: clang-diagnostic-error
    2: bugprone-implicit-widening-of-multiplication-result
    2: bugprone-reserved-identifier
    2: readability-inconsistent-declaration-parameter-name
    2: misc-const-correctness
    2: readability-static-accessed-through-instance
    2: performance-noexcept-move-constructor
    2: clang-analyzer-optin.mpi.MPI-Checker
    2: cppcoreguidelines-prefer-member-initializer
    2: readability-convert-member-functions-to-static
    1: readability-redundant-smartptr-get
    1: readability-suspicious-call-argument
    1: clang-analyzer-core.NonNullParamChecker
    1: performance-move-const-arg
    1: performance-for-range-copy
    1: clang-analyzer-core.CallAndMessage
    1: readability-make-member-function-const

Remember that our CI setup only runs checks on diffs, so I think it is reasonable to address these whenever we touch those respective parts of the code.

Resolves #127.

Clang-tidy 15 includes a new check "misc-const-correctness", which marks locations where local variables could be qualified as const. Unfortunately the check does not seem to be robust enough yet to be enabled as an error (i.e., it includes some false positives). For the time being it is also not checked during CI, as the CI containers currently use Clang 14. Nevertheless, a `clang-tidy --fix` pass has been applied to update many locations to be qualified as const. A workaround has been added to disable the diagnostic on several types (`buffer`, `accessor`, `host_object`, `side_effect`) that could be marked const in most situations, but shouldn't be from a semantic perspective. Additionally, the clang-format configuration now prescribes the position of CV-qualifiers to be to the left of types (i.e., "west-const") through an option that was introduced with Clang 14.

This does not include any fixes as most of the remaining warnings either require some thought to fix or can simply be handled the next time that part of the code is touched.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 55. Check the log or trigger a new build to see more.

examples/distr_io/distr_io.cc

examples/syncing/syncing.cc

include/accessor.h

include/handler.h

src/user_bench.cc

test/accessor_tests.cc

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 30. Check the log or trigger a new build to see more.

test/accessor_tests.cc

test/buffer_manager_tests.cc

test/device_selection_tests.cc

test/runtime_tests.cc

…pass

psalz · 2022-09-27T15:35:13Z

Turns out I was missing two commas in the list of checks...

github-actions

clang-tidy made some suggestions

github-actions · 2022-09-27T16:15:25Z

test/task_graph_tests.cc

 			buf_a.get_access<access_mode::discard_write>(cgh, fixed<1>({64, 64}));
 		});
-		task_id tid_3 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {
+		const task_id tid_3 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {


warning: Value stored to 'tid_3' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

const task_id tid_3 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

test/task_graph_tests.cc:393: Value stored to 'tid_3' during its initialization is never read

const task_id tid_3 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

github-actions · 2022-09-27T16:15:25Z

test/task_graph_tests.cc

 			buf_a.get_access<access_mode::read_write>(cgh, fixed<1>({32, 64}));
 		});

 		auto horizon = task_manager_testspy::get_current_horizon(tm);
 		CHECK(task_manager_testspy::get_num_horizons(tm) == 1);
 		CHECK(horizon.has_value());

-		task_id tid_6 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {
+		const task_id tid_6 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {


warning: Value stored to 'tid_6' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

const task_id tid_6 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

test/task_graph_tests.cc:404: Value stored to 'tid_6' during its initialization is never read

const task_id tid_6 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

github-actions · 2022-09-27T16:15:25Z

test/task_graph_tests.cc

 			buf_b.get_access<access_mode::read_write>(cgh, fixed<1>({0, 128}));
 		});
-		task_id tid_7 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {
+		const task_id tid_7 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {


warning: Value stored to 'tid_7' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

const task_id tid_7 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

test/task_graph_tests.cc:407: Value stored to 'tid_7' during its initialization is never read

const task_id tid_7 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

github-actions · 2022-09-27T16:15:26Z

test/task_graph_tests.cc

@@ -417,7 +417,7 @@
 			CHECK(region_map_a.get_region_values(make_region(32, 96)).front().second.value() == tid_4);
 		}

-		task_id tid_8 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {
+		const task_id tid_8 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) {


warning: Value stored to 'tid_8' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

const task_id tid_8 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

test/task_graph_tests.cc:419: Value stored to 'tid_8' during its initialization is never read

const task_id tid_8 = test_utils::add_host_task(tm, on_master_node, [&](handler& cgh) { ^

fknorr

Nice cleanup! A couple of notes though, mostly about autofixes.

fknorr · 2022-09-28T06:11:50Z

.clang-tidy

+  ##########################
+  # Various check options
+  ##########################
+  - key: misc-const-correctness.TransformPointersAsValues


From the documentation, the warning on pointers should be enabled with WarnPointersAsValues before enabling just the fixit hints with TransformPointersAsValues. Not sure if this is technically required since we are obviously receiving warnings already.

fknorr · 2022-09-28T06:15:44Z

include/accessor.h

+	// The ComputeCpp legacy compiler seems to think that these can throw
+#if CELERITY_DETAIL_IS_OLD_COMPUTECPP_COMPILER
+	accessor(accessor&&) = default;
+	accessor& operator=(accessor&&) = default;
+#else


Does this mean they can't be defaulted? Hm, probably fine as a hack for a compiler whose days are numbered.

fknorr · 2022-09-28T06:20:42Z

include/accessor.h

@@ -221,17 +222,21 @@ class accessor<DataT, Dims, Mode, target::device> : public detail::accessor_base
 	friend struct detail::accessor_testspy;

  public:
+	CELERITY_DETAIL_HACK_CLANG_TIDY_ALLOW_NON_CONST(accessor)


I know this macro was my idea, but maybe we can make the connection between the magic happening here and the explicitly-defaulted constructors more obvious.

Either by renaming it, e.g. CELERITY_DETAIL_HACK_DEFINE_COPY_CTOR_TO_ALLOW_CLANG_TIDY_NON_CONST (that's a mouthful)

Or by just substituting the macro invocations with the actual constructor plus a comment

What do you think?

fknorr · 2022-09-28T06:21:22Z

include/accessor.h

@@ -221,17 +222,21 @@ class accessor<DataT, Dims, Mode, target::device> : public detail::accessor_base
 	friend struct detail::accessor_testspy;

  public:
+	CELERITY_DETAIL_HACK_CLANG_TIDY_ALLOW_NON_CONST(accessor)
+	accessor(accessor&&) noexcept = default;
+	accessor& operator=(accessor&&) noexcept = default;


Consider moving the move-assign definition closer to the copy-assign below (it is not related to the CLANG_TIDY_HACK)

fknorr · 2022-09-28T06:24:52Z

include/accessor.h

@@ -594,6 +614,10 @@ class local_accessor {
 	using const_reference = const DataT&;
 	using size_type = size_t;

+	CELERITY_DETAIL_HACK_CLANG_TIDY_ALLOW_NON_CONST(local_accessor)
+	local_accessor(local_accessor&&) noexcept = default;
+	local_accessor& operator=(local_accessor&&) noexcept = default;


I believe neither this move-assignment nor the existing copy-assignment definitions are actually needed.

fknorr · 2022-09-28T06:44:27Z

test/device_selection_tests.cc

@@ -312,7 +312,7 @@ TEST_CASE_METHOD(celerity::test_utils::mpi_fixture, "pick_device prints expected
 		mp_0.create_devices(dt::cpu);
 		mp_1.create_devices(dt::gpu, dt::gpu);

-		celerity::detail::device_config d_cfg{3, 0};
+		celerity::detail::device_config const d_cfg{3, 0};


Here's an east-const that got away.

fknorr · 2022-09-28T06:44:35Z

test/device_selection_tests.cc

@@ -325,7 +325,7 @@ TEST_CASE_METHOD(celerity::test_utils::mpi_fixture, "pick_device prints expected
 		mp_0.create_devices(dt::cpu);
 		mp_1.create_devices(dt::gpu, dt::gpu);

-		celerity::detail::device_config d_cfg{1, 5};
+		celerity::detail::device_config const d_cfg{1, 5};


Another east const.

fknorr · 2022-09-28T06:45:55Z

test/sycl_tests.cc

@@ -104,7 +104,7 @@ static void test_access(sycl::queue& q, sycl::buffer<int, 1>& test_buf, const su
 		 });
 	 }).wait_and_throw();

-	sycl::host_accessor verify_acc{verify_buf};
+	sycl::host_accessor const verify_acc{verify_buf};


Another east-const.

fknorr · 2022-09-28T06:46:05Z

test/system/distr_tests.cc

@@ -231,7 +231,7 @@ namespace detail {
 					{
 						size_t relative = global_linear_id;
 						for(int nd = 0; nd < Dims; ++nd) {
-							int d = Dims - 1 - nd;
+							int const d = Dims - 1 - nd;


More east constness.

fknorr · 2022-09-28T06:46:43Z

test/test_utils.h

+// Clang-tidy doesn't like that Catch2's RAII helper for scoped INFO messages isn't qualified as `const`.
+// TODO: Fix in Catch2
+#undef INFO
+#define INFO(msg) const INTERNAL_CATCH_INFO("INFO", msg)
+


Is there a way to exclude vendor/* from clang-tidy checks?

psalz added 2 commits September 26, 2022 12:54

clang-tidy: Require _m prefix for protected members as well

fe8f713

clang-tidy: Enable bugprone-* checks

05cf8a3

psalz changed the title ~~clang-tidy: Require _m prefix for protected members as well~~ clang-tidy: Enable more checks Sep 27, 2022

psalz added 2 commits September 27, 2022 12:18

clang-tidy: Enable the whole kitchen sink of checks

1a57677

This does not include any fixes as most of the remaining warnings either require some thought to fix or can simply be handled the next time that part of the code is touched.

psalz force-pushed the enable-more-clang-tidy-rules branch from da7cea0 to 1a57677 Compare September 27, 2022 10:19

github-actions bot reviewed Sep 27, 2022

View reviewed changes

psalz added 3 commits September 27, 2022 17:33

fixup! clang-tidy: Enable (some) misc-* checks; do const-correctness …

73dedf4

…pass

fixup! clang-tidy: Enable the whole kitchen sink of checks

b111680

fixup! clang-tidy: Enable bugprone-* checks

f65e571

github-actions bot reviewed Sep 27, 2022

View reviewed changes

fknorr requested changes Sep 28, 2022

View reviewed changes

psalz mentioned this pull request Feb 20, 2023

clang-tidy: Enable more checks, take 2 #161

Merged

psalz closed this Feb 20, 2023

psalz deleted the enable-more-clang-tidy-rules branch April 6, 2023 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang-tidy: Enable more checks #149

clang-tidy: Enable more checks #149

psalz commented Sep 27, 2022

github-actions bot left a comment

github-actions bot left a comment

psalz commented Sep 27, 2022

github-actions bot left a comment

github-actions bot Sep 27, 2022

github-actions bot Sep 27, 2022

github-actions bot Sep 27, 2022

github-actions bot Sep 27, 2022

fknorr left a comment

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

fknorr Sep 28, 2022

clang-tidy: Enable more checks #149

clang-tidy: Enable more checks #149

Conversation

psalz commented Sep 27, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

psalz commented Sep 27, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 27, 2022

Choose a reason for hiding this comment

github-actions bot Sep 27, 2022

Choose a reason for hiding this comment

github-actions bot Sep 27, 2022

Choose a reason for hiding this comment

github-actions bot Sep 27, 2022

Choose a reason for hiding this comment

fknorr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment