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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

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)
  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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141569