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

Clang tidy #2982

Merged
merged 3 commits into from Dec 1, 2017

Conversation

Projects
None yet
3 participants
@sithhell
Copy link
Member

commented Nov 3, 2017

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
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
},

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

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?

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 3, 2017

Author Member

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.

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

Couldn't args_type be a tuple of references instead?

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 4, 2017

Author Member

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

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 4, 2017

Member

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

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 6, 2017

Author Member

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.

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 6, 2017

Member

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?

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 10, 2017

Author Member

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

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 30, 2017

Author Member

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

This comment has been minimized.

Copy link
@hkaiser

hkaiser Dec 1, 2017

Member

Perfect, thanks!

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

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

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

What is the rationale of this change?

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 3, 2017

Author Member

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.

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

Right, thanks for catching this.

@@ -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());

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

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

This comment has been minimized.

Copy link
@sithhell

sithhell Nov 3, 2017

Author Member

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

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

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

Excellent catch!

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

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 3, 2017

Member

These two are duplicated.

@sithhell sithhell force-pushed the clang_tidy branch from 69463c8 to 34c45ec Nov 3, 2017

@sithhell sithhell force-pushed the clang_tidy branch from 34c45ec to 8e0212d Nov 11, 2017

@biddisco

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

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
Copy link
Contributor

left a comment

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

sithhell added some commits Nov 2, 2017

Improving clang-tidy tool
 - The script now runs more checks.
 - Fixing diff against master
 - Fixing script if no bc is available
Fixing errors reported by clang-tidy
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 sithhell force-pushed the clang_tidy branch from 01f899a to 81f8055 Dec 1, 2017

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2017

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

@hkaiser

hkaiser approved these changes Dec 1, 2017

Copy link
Member

left a comment

LGTM, thanks!

Perfect forward for_loop argumewnts
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 sithhell force-pushed the clang_tidy branch from 81f8055 to 1752889 Dec 1, 2017

@sithhell sithhell merged commit f7e3857 into master Dec 1, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details

@sithhell sithhell deleted the clang_tidy branch Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.