[PATCH] D141569: [clang-tidy] Implement CppCoreGuideline F.18

[Chris Cotter via Phabricator via cfe-commits]

ccotter added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:301
+  }
+  void never_moves(T&& t) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: rvalue reference parameter 't' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]
----------------
ccotter wrote:
> PiotrZSL wrote:
> > that's not always rvalue, if T would be for example int&, then this going to be lvalue...
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141569/new/

https://reviews.llvm.org/D141569