This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement CppCoreGuideline F.18
ClosedPublic

Authored by ccotter on Jan 11 2023, 11:30 PM.

Details

Summary

Warn when an rvalue reference function paramter is never moved
from within the function body.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ccotter requested review of this revision.Jan 11 2023, 11:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 11:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ccotter retitled this revision from Implement CppCoreGuideline F.18 to [clang-tidy] Implement CppCoreGuideline F.18.Jan 11 2023, 11:30 PM
ccotter updated this revision to Diff 488497.Jan 11 2023, 11:52 PM
  • Does not offer fixits

It'd be interesting to see how this handles variadic templates, I have a feeling if it isn't done correctly, there will be a lot of false positives. Can test be added to demonstrate the behaviour.
What happens with code like this

void foo(bar&& B) {
  std::move(B);
}
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
4–36

You can just declare these methods and the base struct, no need to do anything fancy in here and put definitions in there

What happens with code like this

void foo(bar&& B) {
  std::move(B);
}

My new check does not flag this function, although it looks like another check flagged the move expression: ignoring return value of function declared with const attribute [clang-diagnostic-unused-value]. Guideline F.18 only says to flag functions where "the function body uses them without std::move." I think this could be improved with something like "the function body does not move construct or move assign the parameter, and does not pass the result of move() to another function which accepts rvalue references". I can open an issue on the CppCoreGuidelines repo to discuss this. On a related note, I believe there's another clang-tidy check that flags move on const values, so I could see clang-tidy similarly checking other "nonsense" move expressions such as this one (separate check).

ccotter added inline comments.Jan 13 2023, 7:22 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
4–36

Ah, I picked a different test to copy/paste from which does define them 😛. Will fix.

ccotter added inline comments.Jan 13 2023, 7:41 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
4–36

Note, I think all but one existing checks end up defining a more thorough definition of remove_reference:

git grep 'struct remove_ref' | sed -r 's/:.*//' | sort | uniq -c | sort -n
  1 modernize/use-transparent-functors.cpp
  2 cppcoreguidelines/rvalue-reference-param-not-moved.cpp
  3 bugprone/Inputs/unchecked-optional-access/absl/types/optional.h
  3 bugprone/forwarding-reference-overload.cpp
  3 misc/throw-by-value-catch-by-reference.cpp
  3 misc/unconventional-assign-operator.cpp
  3 modernize/use-emplace.cpp
  3 performance/for-range-copy.cpp
  3 performance/move-constructor-init.cpp
  4 bugprone/move-forwarding-reference.cpp
  4 bugprone/use-after-move.cpp
  4 performance/move-const-arg-const-ref.cpp
  4 performance/move-const-arg-trivially-copyable.cpp
  4 performance/move-const-arg.cpp
ccotter updated this revision to Diff 489007.Jan 13 2023, 7:43 AM
  • two more tests
  • Minimize test
ccotter updated this revision to Diff 489174.Jan 13 2023, 6:00 PM
  • Match unresolved calls to move

What happens with code like this

void foo(bar&& B) {
  std::move(B);
}

I raised https://github.com/isocpp/CppCoreGuidelines/issues/2026

ccotter updated this revision to Diff 491124.Jan 21 2023, 9:13 PM
  • Fix windows, fix crash
ccotter updated this revision to Diff 491125.Jan 21 2023, 9:14 PM
  • Fix windows, fix crash
ccotter updated this revision to Diff 491186.Jan 22 2023, 10:18 AM
  • Properly handle forwarding references
ccotter updated this revision to Diff 491197.Jan 22 2023, 12:22 PM
  • Allow move of any expr containing the parameter
carlosgalvezp added inline comments.Jan 24 2023, 5:47 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
14

Not sure if this is allowed in the repo or if we should use something equivalent from ADT.

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h
15–16

Update namespace to C++17

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

You specified C++11 in the LangOpts, but test against C++14 or later in the test, maybe change it here to c++11-or-later?

46

Could we add something like this too?

template <typename T>
struct Foo
{
  void never_moves(T&& t) {}
};

Here t is not a universal reference so the check should warn.

ccotter added inline comments.Jan 24 2023, 6:45 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

The test uses generalize lambda capture, so it needs 14. Should I split the tests that use C++14 features to a separate file, and update this test file to use 11?

ccotter added inline comments.Jan 24 2023, 4:53 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

Ah, and use of auto parameters in other lambdas. So there are a few tests that would need to be split out to a separate file, use an ifdef __cplusplus check (I see one other cppcoreguidelines test does this).

ccotter updated this revision to Diff 491978.Jan 24 2023, 6:16 PM
  • Fix universal reference check; move diagnostic location to parameter name
ccotter updated this revision to Diff 491989.Jan 24 2023, 7:55 PM
  • Use match instead of bespoke traversal
ccotter marked an inline comment as done.Jan 24 2023, 7:57 PM
ccotter added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
14

I was able to replace this with a cleaner solution

ccotter marked an inline comment as done.Jan 24 2023, 8:00 PM

What happens with code like this

void foo(bar&& B) {
  std::move(B);
}

https://github.com/isocpp/CppCoreGuidelines/issues/2026#issuecomment-1402454584 pointed out there's another guideline, ES.56, which a "useless" move such as this example would violate. I imagine this should be a different clang-tidy check though?

carlosgalvezp added inline comments.Jan 24 2023, 8:45 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

Oops I forgot to submit my comment. Yes I think we could keep everything in one test file running c++11-or-later, and #ifdef the C++14 parts.

ccotter added inline comments.Jan 24 2023, 8:59 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

This didn't actually end up working. The test failed looking for expected CHECK-MESSAGES, e.g.,

#if __cplusplus >= 201402L
/* bad code */
// CHECK-MESSAGES
#endif

I'll go the separate file route then?

carlosgalvezp added inline comments.Jan 24 2023, 9:13 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

Hmm right.

What about 2 RUN commands in the same file? One of them you run it for c++11 and the other for c++14-or-later. You can add a check-prefix for the C++14 case, then use CHECK-MESSAGES-SOMEPREFIX such that it only applies to that test invocation. No need to duplicate CHECK-MESSAGES for both runs - if no prefix is specified, it will apply to all runs.

Something like:

#if __cplusplus >= 201402L
/* bad code */
// CHECK-MESSAGES-CXX14
#endif

If it gets too messy maybe a separate test file is the better option, I'm hoping there's not much duplication that needs to be maintaned.

ccotter updated this revision to Diff 491992.Jan 24 2023, 9:29 PM
  • Use multiple runs on the same test file
ccotter marked an inline comment as done.Jan 24 2023, 9:30 PM
ccotter added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
2

Perfect - I didn't realize the test/checker framework was this flexible. Thanks!

ccotter updated this revision to Diff 493004.Jan 28 2023, 7:41 AM
ccotter marked an inline comment as done.

rebase

In https://github.com/isocpp/CppCoreGuidelines/issues/2026, it looks like the core guideline will not permit exceptions for code that accepts an rvalue ref parameter, and the function body moves members of the parameter. So, my moves_member_of_parameter test case would be flagged. In theory, we could add a tool option to allow someone to live more "dangerously" and have the tool accept code like moves_member_of_parameter, with the caveat that the tool (as I have written it so far) would not be smart enough to detect that all static paths move every data member of the object. In any case, I'll update my test and the tool to align exactly with the guideline.

carlosgalvezp added inline comments.Feb 4 2023, 9:20 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
89

Nit: the style convention in LLVM is to not use braces with one-line if/else.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
53

It would be good for the check to specify which parameter it's referring to, for example:

rvalue reference paramter 'o1' is never moved...
ccotter updated this revision to Diff 494850.Feb 4 2023, 2:34 PM
  • Add parameter name to diagnostic
ccotter marked 2 inline comments as done.Feb 4 2023, 2:35 PM

poke - anyone mind reviewing this?

I used this tool to fix two small cases of missing move in the llvm project:
https://reviews.llvm.org/D142824
https://reviews.llvm.org/D142825

carlosgalvezp accepted this revision.Feb 17 2023, 1:19 AM

LGTM thanks! Let's give a couple more days for last reviews. It probably needs a rebase since the Release Notes have got a couple more checks.

This revision is now accepted and ready to land.Feb 17 2023, 1:19 AM
PiotrZSL added a subscriber: PiotrZSL.EditedFeb 17 2023, 6:04 AM

I run this check on project that I work for, here my comments:

False-positives (for me)

std::optional<Type>&& obj + std::move(opj.value());
std::pair<Type2, Type1>&& obj+ std::forward<std::pair<Type2, Type1>>(obj);
Type1&& factory + factory.create() [on initialization list] (create is not && method, but also is not const method, and passing value as & reference would require lvalue as input)
Type&& + member(std:move(obj.member)) [on initialization list]
Type1&& + member(obj) + when Type1 its POD
Type&& + x.emplace_back(std::move(obj.x)); y.emplace_back(std::move(obj.y)); - as function
Type&& (unnamed) + throw UnimplementedException in virtual function
X& operator=(Y&&)  - when we create locally X object and assign it via operator=(X&&), Y used only to access other data
X&& + a = std::move(obj.x), b = std::move(obj.y) (any access to obj member is wraped via std::move, but not all members are moved)
    ^ same as above but some members are just used, for example instead of move std::optional<int> we just copy value (if initialized)
    ^ same on initialization list
XYZ&& as lambda argument (explicit) + std::forward<XYZ>(obj) in lambda with "using XYZ = std::vector<..>".
X&& obj as argument + for(auto& v : obj) { something(std::move(v)); }
X&& obj as argument + for(auto& v : obj) { something(std::move(v.x)); } 
X&& obj (as function argument)+ lambda called in function [&](auto& v) { v.x = std::move(obj.x); }
std::optional<std::vector<std::uint8_t>>&& obj + xyz.push_back({other.a, other.b, std::move(obj), other.c}); - vector type has no constructor, fields initialization
container&& from + for(auto&& value : from) { if (x) { a = std::move(value); } else {dest.emplace_back(std::move(value)); } }
const Type&& obj + std::move(obj) in function
FlatMap&& obj + if (const auto it = obj.find(xyz)) { something = std::move(it->second); } + obj.clear() at end
template<X> SomeType serialize(const X&, OtherType&&, bool isSomething); (in header, private template function declaration with definition in cpp that also got warning)
template <typename U>   Type& operator=(std::shared_ptr<U>&& u) {  assign(std::forward<U>(u));   return *this; }
template<typename U> void XYZ::swap(XYZ&& other) { something.reset(); other.something.reset(); T::swap(other); (T is template parameter, for example std::map);
Map&& xyz, auto&& found = xyz.find(value); if (found != xyz.cend()) { something.merge(found->second); }
Map&& xyz, for(auto& [x,y] : xyz) { something.merge(y); }:
constructor: explicit Something(T&& value) : value(std::forward<T>(value)) {}, where T is class template argument

Overall 167 issues, half false-positive, but I already use other check for adding std::move, still had some good finding.
For some of the issues message is wrong, for example it could suggest moving some members (that were not moved) or use std::move instead of std::forward.
But for both cases probably other (new) checks could be better.

In summary:

  • does not understand std::forward
  • does not understand partial std::move
  • does not understand ::swap and ::merge
  • does not understand unused (unnamed) arguments
  • some of use cases are && on purpose, just to hint user that passed value should be temporary, with const & it would accept also lvalue, such variables usually are created in place via constructor call or just initialization with {x, y, z}

I would say, do check more restricted, better is find less issues, than raising warnings on correct use-cases.

Note that I run this on top of Clang 15.

Some of the examples you mentioned are indeed not compliant with guideline F.18. I have a test for at least one of the examples you gave (moves_deref_optional matches your first example). The guideline is fairly strict IMO, and I asked about this in https://github.com/isocpp/CppCoreGuidelines/issues/2026, but the response was that the guideline will remain with the requirement that the whole object needs to be moved. This would imply that the fix for cases like moves_deref_optional would be to change the parameter to be a value rather than rvalue reference of the type (and move the individual parts as desired). Example X&& obj as argument + for(auto& v : obj) { something(std::move(v.x)); } also violates F.18 since the whole object is not moved (just a subobject, although I'm a little confused about the loop also).

One example (second, std::pair<Type2, Type1>&& obj+ std::forward<std::pair<Type2, Type1>>(obj);) seems invalid or incomplete, since std::pair<Type2, Type1> is not a universal reference and shouldn't be forwarded. I couldn't follow the other forward examples. In your swap example, this code also confused me - could you clarify it (it looks like the tool would be correct since I don't see any move of the parameter other).

const Type&& obj + std::move(obj) in function - this is a good catch, I don't have a test for this, but my tool incorrectly flags this. I will fix this.

All that said, I did consider but not implement an option to the tool which would allow a less strict version of F.18 which considered moves of any expression containing the parameter to be considered a move of the object. E.g., both of my tests moves_member_of_parameter and moves_deref_optional violate F.18, although I can reasonably see code written this way with the author not desiring this check to flag such code (of course, it could be compliant with the guideline by accepting a value rather than rvalue ref of the object). I recall seeing discussion elsewhere on a phab that clang-tidy tends to add options to allow less strict interpretations of the guidelines. I think it'd be worth adding this specific option. Should I add it as default enabled or disabled (i.e., should the default be strict adherence or not)?

I think we aim at having it strict as default (so someone wanting to comply with the rule "as-is" can just enable the check), and then add options to relax it.

PiotrZSL added a comment.EditedFeb 17 2023, 12:24 PM

You right swap case can also be invalid (rvalue not needed, there).
Anyway it it has too be strict then this can be strict.

But message could be better, for example if we don't move all fields of class, then this could warn that its not fully moved, or that some members are accessed without move., or at least is not moved as whole.
Because now it could be confusing...

void Class:set(Obj&& obj) // warning: rvalue reference parameter 'obj' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]
{
    this->a = std::move(obj.a);
    this->b = std::move(obj.b);
    this->c = std::move(obj.c);
}

And then when you read "is never moved" you tap your forehead, about whats going on, you did move everything you wanted.

@PiotrZSL The last forward example you have should match my does_move_auto_rvalue_ref_param test, which is not flagged by the tool. Let me know if you have any other forward cases (preferred as full self contained examples) that the tool incorrectly flags, as getting forward / universal references cases are important for this tool.

ccotter updated this revision to Diff 498525.Feb 17 2023, 4:55 PM
  • Add StrictMode option, fix const&& bug
ccotter updated this revision to Diff 498526.Feb 17 2023, 4:56 PM
  • Add back -fno-delayed-template-parsing to test
ccotter marked an inline comment as done.Feb 17 2023, 5:02 PM

Rebased as well on top of the latest release notes

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
46

Done in the AClassTemplate::never_moves test.

ccotter updated this revision to Diff 498528.Feb 17 2023, 5:04 PM
ccotter marked an inline comment as done.Feb 17 2023, 5:04 PM

rebase again

@PiotrZSL I added an option to allow disabling the strict behavior. It should address many of your examples (e.g., moving subobjects) . Let me know if you have more feedback.

@PiotrZSL I added an option to allow disabling the strict behavior. It should address many of your examples (e.g., moving subobjects) . Let me know if you have more feedback.

I will re-test this on Monday/Tuesday. and comeback to you if I find something.

ccotter updated this revision to Diff 498730.Feb 19 2023, 10:50 PM
  • Add back -fno-delayed-template-parsing to test

I don't know why but with latest version I still got false positive with std::function on Clang 15.

using Functor= std::function<... something ...>;

std::unique_ptr Class::createSomething(::Functor&& functor) {
   return std::make_unique<SomeType>(std::move(functor));
}
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
42

duplicated isDefinition and ToParam

Other false-positive that I still see:

void SomeClass::someVirtualFunction(SomeType&&)
{
BOOST_THROW_EXCEPTION(std::runtime_error("Unexpected call ...."));
}

It shouldnt warn about virtual functions with unnamed parameter, because this can be required by interface.

And on something like this I still got reproduction in production code:

template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};
PiotrZSL added inline comments.Feb 20 2023, 7:25 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
302

that's not always rvalue, if T would be for example int&, then this going to be lvalue...

ccotter updated this revision to Diff 498873.Feb 20 2023, 8:38 AM
  • Remove duplicated matchers
ccotter marked an inline comment as done.Feb 20 2023, 8:38 AM
ccotter added a comment.EditedFeb 20 2023, 8:50 AM

In

template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};

T is not a universal reference in the constructor, it's an rvalue reference to T. There is no deducing context, so universal references are not involved here (and, std::forward would also be incorrect here). The following would be a deducing context with a universal reference:

template <int tagValue, typename T>
struct SomeClass
{
public:
  template <class T2>
  explicit SomeClass(T2&& value) : value(std::forward<T2>(value)) {}
 T value;
};

I thought there was a clang-tidy check that flagged applying std::forward on a non-universal reference, but I don't see one.

ccotter added a comment.EditedFeb 20 2023, 8:57 AM

I was split on handling the case where the parameter that was never moved from was not named. For this guideline enforcement to flag all unmoved rvalue reference parameters, code that std::moves into an argument to a virtual method call could be especially confusing or dangerous with regards to whether the object was truly "moved" (move constructor/assignment invoked within the method call). E.g.,

LargeObject obj;
ClassWithVirtualMerthods* v = ...;
v->some_virtual_call(std::move(obj));

If would be confusing if some implementations of the virtual method actually moved the parameter, while others didn't. I'm inclined in this case (regardless of whether the parameter is named in a virtual overridden method) to still flag this code. Looking at the rule enforcement criteria again, the rule states "Don’t conditionally move from objects" which this case falls into.

I could see there being an option for this specific case (defaulted off), although it's very specific of a case, and for something I think the rule should be especially attentive towards. Could I get some additional feedback on this specific case?

ccotter added inline comments.Feb 20 2023, 9:02 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
302

Looking at the rule enforcement, I might have overlooked one thing:

> Flag all X&& parameters (where X is not a template type parameter name) where the function body uses them without std::move.

I think this means this code should not be flagged, even if T is an object which is cheap to move but expensive to copy (and not a reference to that type). I would like for AClassTemplate<LargeObject> to be flagged, but not AClassTemplate<LargeObject&>, so let me look into this to see if I can limit this to instantiations of the type, rather than looking at the template definition itself.

In

template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};

T is not a universal reference in the constructor, it's an rvalue reference to T. There is no deducing context, so universal references are not involved here (and, std::forward would also be incorrect here). The following would be a deducing context with a universal reference:

Wrong:

#include <utility>

template<typename T>
struct Wrap {

  Wrap(T&& arg) 
    : value(arg)
  {}

  T value;  
};

struct Value {};

int main()
{
    Value value;
    Wrap<Value> v1(std::move(value));
    Wrap<Value> v2(std::move(value));
    Wrap<Value&> v3(value);
    return 0;
}

If you add std::move you will get compilation error, if you add std::forward, everything will work fine. Simply Value& and && will evaluate to Value&, no rvalue reference here.

I was split on handling the case where the parameter that was never moved from was not named. For this guideline enforcement to flag all unmoved rvalue reference parameters, code that std::moves into an argument to a virtual method call could be especially confusing or dangerous with regards to whether the object was truly "moved" (move constructor/assignment invoked within the method call). E.g.,

LargeObject obj;
ClassWithVirtualMerthods* v = ...;
v->some_virtual_call(std::move(obj));

If would be confusing if some implementations of the virtual method actually moved the parameter, while others didn't. I'm inclined in this case (regardless of whether the parameter is named in a virtual overridden method) to still flag this code. Looking at the rule enforcement criteria again, the rule states "Don’t conditionally move from objects" which this case falls into.

I could see there being an option for this specific case (defaulted off), although it's very specific of a case, and for something I think the rule should be especially attentive towards. Could I get some additional feedback on this specific case?

I'm not so sure, you may have Base class Car that would have void changeBackRightDoor(Door&&) method, and then you could have class Car5Doors and Car3Doors, in such case its easy to known that Car3Doors wouldn't implement changeBackRightDoor method, and that method wouldnt move anything, using unnamed parameters is a way to go. If you dont exclude this, then at least provide option for that, like IgnoreUnnamedParameters.

ccotter updated this revision to Diff 499034.Feb 20 2023, 8:41 PM
  • Ignore params of template type
  • Add option for unnamed params
> If you add std::move you will get compilation error, if you add std::forward, everything will work fine. Simply Value& and && will evaluate to Value&, no rvalue reference here.

I agree that examining the template definition alone is not correct. In your original example with SomeClass, T is not a forwarding reference since T is not deduced in the constructor, so it's misleading to use forward in this context. forward "works" and leads to the behavior you are likely seeking in your SomeClass example, but not in the usual way the most readers would expect (the spirit of the CppCoreGuidelines is to provide certain restricted ideas as recommendations, and using forward in this way violates ES.56: "Flag when std::forward is applied to other than a forwarding reference."). Usually, I'd expect to see a class template on a type to have different specializations for T being a non-reference and another for T being a reference (e.g., std::future), or only allow T to be a reference or non-reference (e.g., std::optional), but not both.

ccotter marked 4 inline comments as done.Feb 20 2023, 8:54 PM
ccotter added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
46

@carlosgalvezp Note I've changed the check to not flag never_moves based on more recent discussion. I do think this code should flag, although I'm a little torn. If Foo constrains T to be a reference, then Foo<Obj&>::never_moves is "OK", although I think it's still confusing as never_moves could instead accept T&.

I'll need to rethink this a bit, but I've never the test in place with a FIXME and brief comment explaining the situation.

ccotter added inline comments.Feb 20 2023, 9:29 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
46

That should read "I've left the test in place"

PiotrZSL added inline comments.Feb 21 2023, 6:12 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
22

and you dont need allOf

23–25

those lines should be equal to just: parmVarDecl(hasType(type(rValueReferenceType()))).bind("param"),
i'm not sure why you need additional equalsBoundNode, thats useful mainly if you would do some tricks with hasParent or something...

26–30

you could try to merge those 3:

unless(hasType(qualType(references(anyOf(isConstQualified(), templateTypeParmType(hasDeclaration(templateTypeParmDecl())), substTemplateTypeParmType())))
41

you already got constructor as second in any of, it shouldnt be matched here, so this line shouldnt be needed

47

you dont need to split things like std::move or lambda detections.

you can add this right to matcher, like:

unless(hasDescendant(lambdaExpr(hasAnyCapture(capturesVar(equalsBoundMode("param"), ...

and here you may need to add new matcher to match ByCopy))
but you dont need to check lambdas in this way, you dont need to check them anyway, they should be check as descendants of functions

104–126
unless(hasDescendant(callExpr(
      anyOf(callee(functionDecl(hasName("::std::move"))),
            callee(unresolvedLookupExpr(hasAnyDeclaration(
                namedDecl(hasUnderlyingDecl(hasName("::std::move"))))))),
      argumentCountIs(1), hasArgument(0, moveArgumentOf(StrictMode, Param)))))

any here you could also check lambdas, if you want to ignore copies, but not should why you should do that, but you can always bind this to "move", or implement lambda checking as matcher.... unless(hasAncestor(lambdaExpr(isCapturedByCopy....

Simply as you have some tests already, try pack as much as you can into registerMatchers, because once you split this across multiple submatches in diffrent functions, then it could be hard to maintain.

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h
33–34

use bool here, or it could cause some issues, you wont save anything by using bits. class alignment is already 8.
problem is that you read bools but in storeOptions you store ints, and when dump-config will be used it will show 1/0 not true/false.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst
2

add explicit documentation about options

14

one example

PiotrZSL added inline comments.Feb 21 2023, 6:36 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
27–30

if with this you want to exclude parameters that depend on templates, consider looking into isTemplateInstantiation AST matcher, other option could be to get DeclContext and check isDependentContext(). You would need to check this, maybe look into other checks how its done there. Simply because if this would be for example T* &&, then it could not be detected.

ccotter updated this revision to Diff 499341.Feb 21 2023, 6:23 PM
ccotter marked 4 inline comments as done.
  • Simplify matcher

Simplified matchers

ccotter added inline comments.Feb 21 2023, 6:30 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h
33–34

Ah, I happened to see ArgumentCommentCheck.h which seems to be lone check that uses bits...

ccotter updated this revision to Diff 499350.Feb 21 2023, 7:02 PM
  • Use bool
  • Combine into a single matcher
ccotter marked 2 inline comments as done.Feb 21 2023, 7:02 PM
ccotter added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
104–126

Good call. Single matcher expr is cleaner.

ccotter updated this revision to Diff 499363.Feb 21 2023, 8:37 PM
ccotter marked an inline comment as done.
  • Finish combining into a single matcher
  • Rename option and add docs
ccotter added inline comments.Feb 21 2023, 8:43 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
302

Thinking about this more, the enforcement spec for F.18 does apply to the SomeClass example (shown below again), since T is not a deduced parameter in the context of the constructor. Indeed, it has generated a bit of discussion which I think is the specific point of the guideline to avoid confusion around the non-trivial but important concepts of rvalue references, universal references, move/forwards, etc (and, again, I think one would usually expect to see different specializations for T vs T& and/or disallowing one of T/T& to avoid confusion to the reader or even author).

template <class T>
struct SomeClass {
    SomeClass(T&&) { ... } // T is not deduced.
};

I can add an option (although I'm a little worried now as there will be 3 options for this check, which seems like a bit much) for this with the default to adhere to the guideline as stated.

ccotter updated this revision to Diff 499366.Feb 21 2023, 9:27 PM
  • Include non-deduced template types
ccotter marked 5 inline comments as done.Feb 21 2023, 9:28 PM

Ok, all feedback should be addressed (thanks for the comment to consolidate into a single matcher) with appropriate options to relax the enforcement of the guideline.

Overall not bad, except reported things, I don't have any complains.
Number of options is not an issue.
90% of users wont use them, 10% will be happy to find them instead of dealing with NOLINT.

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
52

i thing you dont need here "hasDescendant(declRefExpr(equalsBoundNode("ref"))),"
simply because hasAncestor alone is doing a trick, if lambda is ancestor, then this call is a descendant for it.

57–58

consider:

hasType(type(rValueReferenceType())),
decl().bind("param"),
optionally(hasType(
                  qualType(references(templateTypeParmType(hasDeclaration(
                      templateTypeParmDecl().bind("template-type"))))))),

no need to duplicate parmVarDecl

62

put this unless before optionally...

65–75

looks like lambda context is visible as both operator() and as body to lambaExpr directly.
this mean that it may match twice, once as functionDecl, and once as lambdaExpr.
You can merge functionDecl with cxxConstructorDecl, just do same trick like with isMoveAssignmentOperator.

I would probably start with functionDecl, and then would try do some trick like forEachParam, but we dont have such matcher....
You missing also isDefinition() in functionDecl even that you got hasBody.

111

consider style:
if (MoveCall) return;

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst
25

maybe AllowPartialMove

ccotter updated this revision to Diff 499681.Feb 22 2023, 5:21 PM
ccotter marked 4 inline comments as done.
  • More matcher simplification
ccotter marked 4 inline comments as done.Feb 22 2023, 5:21 PM
ccotter added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
65–75

I don't remember why I ever had lambda specific logic here, but it doesn't seem necessary.

ccotter marked an inline comment as done.Feb 22 2023, 5:22 PM
ccotter added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp
111

I left this as is as I recall seeing on other phabs a preference towards keeping the curly braces for expressions that (after taking into account line length style rules) span multiple lines. I think it helps my own readability in these situations.

PiotrZSL added a comment.EditedFeb 23 2023, 3:42 AM

One more thing I noticed, this check can be in conflict with performance-move-const-arg.

warning: std::move of the variable 'xyz' of the trivially-copyable type 'types::xyz' has no effect; remove std::move() [performance-move-const-arg]

so would be nice to make sure that trivially-copyable types are ignored., or add option to ignore them

Trivial types should not be passed by rvalue reference, but by value per the diagram under http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing. I feel like adding an option to opt-out of this is missing the point of this check, and rvalue references of trivial type should just be fixed rather than adding an option to permit them.

Trivial types should not be passed by rvalue reference, but by value per the diagram under http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing. I feel like adding an option to opt-out of this is missing the point of this check, and rvalue references of trivial type should just be fixed rather than adding an option to permit them.

Imaginate that such trivial type could be for example 200KB in size, copy to argument, and then from argument to for example class member could be costly. Using rvalue allows to pass non const object to some for example function.
I'm asking about option, just because other check also got such option, and if both will have one, then user can decide whatever he want to use std::move or not.
Still I agree that because we don't lose anything there, moving such trivial variable using std::move still would be fine, and at that point checking trivial variables in other check could be disabled, that's what I did in my project what I was fixing these issues.
So all is up to you.

If else, then maybe it would be good to deliver this check, if something happen then before end of release it still could be fixed.

> Imaginate that such trivial type could be for example 200KB in size

This should be passed by const ref then correct (listed under "Expensive to move (e.g. big BigPOD[]" in the parameter passing guidelines) ? Are there cases where a big pod type needs to be passed by rvalue ref, without std::move where const ref cannot be used instead?

ccotter updated this revision to Diff 500648.Feb 26 2023, 6:50 PM
ccotter marked an inline comment as done.
  • More clean option name
ccotter marked an inline comment as done.Feb 26 2023, 6:52 PM

Thanks for the name suggestion

bump. I never heard back on the case where using an rvalue reference for a big pod type as opposed to const ref.

bump. I never heard back on the case where using an rvalue reference for a big pod type as opposed to const ref.

I have such use cases in project, they not necessary POD, but more like a way to wrap many arguments into single class.
Something like this:

struct A
{
    void call();  
};

struct Parameters
{
    A subclass;
    // here bunch of other members
};

void function(Parameters&& p)
{
    p.subclass.call();
}

void test()
{
    A a;
    function({a});
}

When you change this into normal lvalue reference, then it wont compile, when you add const then it also wont compile.
In this cases I would probably used //NOLINT, so don't consider this case for this check as blocker.

For POD this issue also may happen but it's related more to the scenario when function modify input data by exploiting fact that input is not const.
In such case you may for example prepare message that would be created by using {}, then call function that would take it as rvalue, function could fill up some additional data like translation id, and send it.
By using rvalue you telling that function that it is an owner of this object now and can do anything with it, including changing it, but not necessarily need to move it.
But still this is also probably use case for NOLINT.

I was thinking... In theory fix for this check should be one of: use const &, use value, move rvalue.
But const & is not always in an option, in utils we got 2 functions: isOnlyUsedAsConst, isExpensiveToCopy.
So in theory if variable can be used as const, then const & can be used, if var cannot be used as const and is cheap to copy, then in theory value can be used, otherwise we deal with something intended (like partial move, or write to variable), and such cases could be +- ignored in relaxed mode.

Still, this is just something to thing, not necessarily to implement.

@ccotter Do we commit this, or you plan to still change something here ?

Yes, I'm ready for this to be committed. I don't have commit access, but if anyone could commit this on my behalf with Chris Cotter <ccotter14@bloomberg.net> that'd be great!

PiotrZSL updated this revision to Diff 504454.Mar 12 2023, 10:27 AM

Rebase + Fix order of checks in ReleaseNotes

This revision was landed with ongoing or failed builds.Mar 12 2023, 10:27 AM
This revision was automatically updated to reflect the committed changes.