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

[Chris Cotter via Phabricator via cfe-commits]

ccotter added a comment.

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 `move`s 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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141569