This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Remove duplicated check from move-constructor-init
ClosedPublic

Authored by malcolm.parsons on Nov 9 2016, 8:38 AM.

Details

Summary

An addition to the move-constructor-init check was duplicating the
modernize-pass-by-value check.
Remove the additional check and UseCERTSemantics option.
Run the move-constructor-init test with both checks enabled.
Fix modernize-pass-by-value false-positive when initializing a base
class.
Add option to modernize-pass-by-value to only warn about parameters
that are already values.

Diff Detail

Repository
rL LLVM

Event Timeline

malcolm.parsons retitled this revision from to [clang-tidy] Remove duplicated check from move-constructor-init.
malcolm.parsons updated this object.
malcolm.parsons added a subscriber: cfe-commits.
flx edited edge metadata.Nov 9 2016, 8:42 AM

Is the modernize-pass-by-value check configurable in a way to only trigger when copied constructor arguments are not moved?

I think our use case has been to have move-constructor-init check enabled to catch cases that can be optimized but not trigger on every constructor that uses const& instead of value + move, i.e. modernizing is not prescribed, but catching cases where the argument is already passed by value and making them faster is desirable.

In D26453#590636, @flx wrote:

Is the modernize-pass-by-value check configurable in a way to only trigger when copied constructor arguments are not moved?

No; good idea.

malcolm.parsons edited edge metadata.

Update comment in performance-unnecessary-value-param check.

malcolm.parsons updated this object.

Add ValuesOnly option to modernize-pass-by-value.

Store new option.

flx added a comment.Nov 11 2016, 9:14 AM

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

I think this meets my needs for the cert-oop11-cpp check as it appears to be preserving the original behavior. If a user has the UseCERTSemantics option set in one of their scripts, that appears to be silently accepted and discarded currently, so I don't think this will break anyone's build scripts.

The downside has more to do with the categorization. Not everyone enables "modernize", which is now the only place to be told about a possible correctness issue that misc-move-constructor-init was previously reporting. (This part of the check is one half modernization, one half performance, and one half correctness, depending on which lens you view the code through.) I don't think that's a huge downside, however.

This change should definitely be called out in the release notes.

(This part of the check is one half modernization, one half performance, and one half correctness, depending on which lens you view the code through.)

The performance part should be handled by performance-unnecessary-value-param.
It should warn in this situation, but not create a fix that would conflict with modernize-pass-by-value's fix.

malcolm.parsons edited edge metadata.

Mention in release notes.

In D26453#592934, @flx wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

ping.

In D26453#592934, @flx wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

ping.

@alexfh ping.

aaron.ballman accepted this revision.Dec 16 2016, 1:22 PM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Dec 16 2016, 1:22 PM
This revision was automatically updated to reflect the committed changes.
alexfh edited edge metadata.Dec 21 2016, 7:17 AM

LG.

In D26453#592934, @flx wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

Yep, I think so.

clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp
1

Test for one check running a different check is somewhat confusing. If we need to ensure that the patterns the two checks target don't overlap, maybe we should rename the test (<check1>+<check2>.cpp or something like that)?

D28022 changes performance-unnecessary-value-param so that it handles this part of modernize-pass-by-value.
So this isn't the end of the story here yet.

clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp
1

Doing it like this was to show that no functionality was lost, not that there's no overlap.
The pass-by-value tests can be moved somewhere else.