Clang tidy #2982

sithhell · 2017-11-03T12:08:10Z

Fixes as reported by clang-tidy after enabling the following checks:

modernize-use-nullptr
misc-use-after-move
misc-virtual-near-miss
misc-multiple-statement-macro
misc-move-constructor-init
misc-move-forwarding-reference
misc-assert-side-effect
misc-dangling-handle
misc-move-constructor-init
misc-move-forwarding-reference
misc-non-copyable-objects
misc-forwarding-reference-overload

hkaiser · 2017-11-03T12:27:17Z

hpx/parallel/algorithms/for_loop.hpp

                return util::partitioner<ExPolicy>::call_with_index(
                    policy, first, size, stride,
                    part_iterations<F, S, args_type>{
-                        std::forward<F>(f), stride, std::move(args)
+                        std::forward<F>(f), stride, args
                    },


AFAICS, this code causes the data to be copied twice. Once when creating args and once while calling call_with_index. Is there any way to avoid that?

Yes, that's a potential problem indeed. I don't know how to avoid that. args is needed twice, once for part_iterations and then in the finalizer.

Couldn't args_type be a tuple of references instead?

No, they need to decay-copied at least once to stay in scope long enough.

Ok, so let's copy them once and not thrice.

I wouldn't know how...
The first one, is forwarding the arguments into a local tuple, which then needs to be passed to the iteration function and the exit function, which both need a copy of the arguments.

Well, the exit function is guaranteed to outlive everything else, right? So why not hold the copy there and let the iteration refer to those copies? Would that work?

Hmm, this would require some significant refactoring of the partitioner. I would prefer doing that in another PR though, but I guess it would work in principle

Ok, the issue of having multiple copies should be fixed with the latest commit.

Perfect, thanks!

hkaiser · 2017-11-03T12:33:40Z

src/runtime/agas/server/route.cpp

@@ -75,6 +75,7 @@ namespace hpx { namespace agas { namespace server
            addr.address_ = g.lva();
        }

+        naming::id_type source = p.source_id();


What is the rationale of this change?

p is moved here: https://github.com/STEllAR-GROUP/hpx/pull/2982/files/69463c83e47825205fbb10147b2d97b6726e1242#diff-fe4bb9d1c3c7a064a0b4dd4bdae63198L87

That means that the source_id used later on to update the cache entry is being used from a moved from object.

Right, thanks for catching this.

hkaiser · 2017-11-03T12:36:26Z

src/runtime/resource/detail/detail_partitioner.cpp

@@ -505,7 +505,6 @@ namespace hpx { namespace resource { namespace detail
        affinity_data_.set_num_threads(new_pu_nums.size());
        affinity_data_.set_pu_nums(std::move(new_pu_nums));
        affinity_data_.set_affinity_masks(std::move(new_affinity_masks));
-        affinity_data_.init_cached_pu_nums(new_pu_nums.size());


What is the rationale of this change? is that related to the clang-tidy checks?

Yes, new_pu_nums was moved a few lines above. When looking at the code of affinity_data, init_cached_pu_nums does nothing in this specific case anyway: https://github.com/STEllAR-GROUP/hpx/blob/master/src/runtime/threads/policies/affinity_data.cpp#L324

hkaiser · 2017-11-03T12:37:05Z

src/runtime/threads/threadmanager.cpp

@@ -286,7 +286,7 @@ namespace hpx { namespace threads
            "default",
            "low",
            "normal",
-            "high (recursive)"
+            "high (recursive)",


Excellent catch!

hkaiser · 2017-11-03T13:03:58Z

tools/clang-tidy.sh

+CHECKS="$CHECKS,misc-assert-side-effect"
+CHECKS="$CHECKS,misc-dangling-handle"
+CHECKS="$CHECKS,misc-move-constructor-init"
+CHECKS="$CHECKS,misc-move-forwarding-reference"


These two are duplicated.

biddisco · 2017-11-30T13:56:54Z

This PR looks like it should be merged. I will create an Issue to remember the partitioner copy problem and then someone can/should merge this.

biddisco

LGTM. I have created a seperate issue for the arg copy discussion

- The script now runs more checks. - Fixing diff against master - Fixing script if no bc is available

This patch fixes problems reported by clang-tidy after enabling the following checks: modernize-use-nullptr misc-use-after-move misc-virtual-near-miss misc-multiple-statement-macro misc-move-constructor-init misc-move-forwarding-reference misc-assert-side-effect misc-dangling-handle misc-move-constructor-init misc-move-forwarding-reference misc-non-copyable-objects misc-forwarding-reference-overload

sithhell · 2017-12-01T09:44:25Z

All comments have been addressed. This should be good to go now.

hkaiser

LGTM, thanks!

This patch enables the partitioner to accept parameters which are perfect forwarded to the passed iteration functions when used with call_with_index. This properly fixes the for_loop related use-after-move warnings reported by clang-tidy.

hkaiser added category: buildbot category: core category: diagnostics type: enhancement labels Nov 3, 2017

hkaiser added this to the 1.1.0 milestone Nov 3, 2017

hkaiser reviewed Nov 3, 2017

View reviewed changes

sithhell force-pushed the clang_tidy branch from 69463c8 to 34c45ec Compare November 3, 2017 13:24

sithhell force-pushed the clang_tidy branch from 34c45ec to 8e0212d Compare November 11, 2017 20:14

biddisco approved these changes Nov 30, 2017

View reviewed changes

sithhell force-pushed the clang_tidy branch from 8e0212d to b9bde4b Compare November 30, 2017 14:17

biddisco mentioned this pull request Nov 30, 2017

Fix partitioner arg copy found in #2982 #3032

Closed

Thomas Heller added 2 commits December 1, 2017 10:40

Improving clang-tidy tool

15b24b6

- The script now runs more checks. - Fixing diff against master - Fixing script if no bc is available

sithhell force-pushed the clang_tidy branch from 01f899a to 81f8055 Compare December 1, 2017 09:43

hkaiser approved these changes Dec 1, 2017

View reviewed changes

Perfect forward for_loop argumewnts

1752889

This patch enables the partitioner to accept parameters which are perfect forwarded to the passed iteration functions when used with call_with_index. This properly fixes the for_loop related use-after-move warnings reported by clang-tidy.

sithhell force-pushed the clang_tidy branch from 81f8055 to 1752889 Compare December 1, 2017 13:29

sithhell merged commit f7e3857 into master Dec 1, 2017

sithhell deleted the clang_tidy branch December 1, 2017 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang tidy #2982

Clang tidy #2982

sithhell commented Nov 3, 2017

hkaiser Nov 3, 2017

sithhell Nov 3, 2017

hkaiser Nov 3, 2017

sithhell Nov 4, 2017

hkaiser Nov 4, 2017

sithhell Nov 6, 2017

hkaiser Nov 6, 2017

sithhell Nov 10, 2017

sithhell Nov 30, 2017

hkaiser Dec 1, 2017

hkaiser Nov 3, 2017

sithhell Nov 3, 2017

hkaiser Nov 3, 2017

hkaiser Nov 3, 2017

sithhell Nov 3, 2017

hkaiser Nov 3, 2017

hkaiser Nov 3, 2017

biddisco commented Nov 30, 2017

biddisco left a comment

sithhell commented Dec 1, 2017

hkaiser left a comment

Clang tidy #2982

Clang tidy #2982

Conversation

sithhell commented Nov 3, 2017

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

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

biddisco commented Nov 30, 2017

biddisco left a comment

Choose a reason for hiding this comment

sithhell commented Dec 1, 2017

hkaiser left a comment

Choose a reason for hiding this comment