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

ccotter added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc491c9170239: [clang-tidy] Implement CppCoreGuideline F.18 (authored by ccotter, committed by PiotrZSL). Repository: rG LLVM Github Monorepo

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

PiotrZSL updated this revision to Diff 504454. PiotrZSL added a comment. Rebase + Fix order of checks in ReleaseNotes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added a comment. 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 ` that'd be great! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/

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

PiotrZSL added a comment. @ccotter Do we commit this, or you plan to still change something here ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 ___

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

PiotrZSL added a comment. 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 &

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

PiotrZSL added a comment. In D141569#4163607 , @ccotter wrote: > 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

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

ccotter added a comment. bump. I never heard back on the case where using an rvalue reference for a big pod type as opposed to const ref. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569

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

ccotter marked an inline comment as done. ccotter added a comment. Thanks for the name suggestion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 ___

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

ccotter updated this revision to Diff 500648. ccotter marked an inline comment as done. ccotter added a comment. - More clean option name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added a comment. > 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

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

PiotrZSL added a comment. In D141569#4147268 , @ccotter wrote: > 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

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

ccotter added a comment. 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

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

PiotrZSL added a comment. 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

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

ccotter marked an inline comment as done. ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:110 + const auto *MoveCall = Result.Nodes.getNodeAs("move-call"); + if (!MoveCall) { +

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

ccotter marked 4 inline comments as done. ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:64-74 + hasAncestor(compoundStmt(hasParent(lambdaExpr( +

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

ccotter updated this revision to Diff 499681. ccotter marked 4 inline comments as done. ccotter added a comment. - More matcher simplification Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

PiotrZSL added a comment. 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. Comment at:

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

ccotter added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

ccotter updated this revision to Diff 499366. ccotter added a comment. - Include non-deduced template types Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

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

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

ccotter updated this revision to Diff 499363. ccotter marked an inline comment as done. ccotter added a comment. - Finish combining into a single matcher - Rename option and add docs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/

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

ccotter marked 2 inline comments as done. ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:103-125 + StatementMatcher MoveCallMatcher = callExpr( +

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

ccotter updated this revision to Diff 499350. ccotter added a comment. - Use bool - Combine into a single matcher Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h:32-33 +private: + const unsigned StrictMode : 1; + const unsigned IgnoreUnnamedParams : 1; +}; PiotrZSL wrote: > use bool here, or it

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

ccotter added a comment. Simplified matchers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ccotter updated this revision to Diff 499341. ccotter marked 4 inline comments as done. ccotter added a comment. - Simplify matcher Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:26-29 + unless(hasType(qualType(references(templateTypeParmType( + hasDeclaration(templateTypeParmDecl())), +

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:21 + Finder->addMatcher( + parmVarDecl(allOf( + parmVarDecl(hasType(type(rValueReferenceType(.bind("param"), and

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:22-24 + parmVarDecl(hasType(type(rValueReferenceType(.bind("param"), + parmVarDecl( + equalsBoundNode("param"),

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

ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:45 +template +void never_moves_param_template(Obj&& o, T t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: rvalue reference parameter

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

ccotter marked 4 inline comments as done. ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:45 +template +void never_moves_param_template(Obj&& o, T t) { + // CHECK-MESSAGES:

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

ccotter added a comment. > 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

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

ccotter updated this revision to Diff 499034. ccotter added a comment. - Ignore params of template type - Add option for unnamed params Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

PiotrZSL added a comment. In D141569#4139320 , @ccotter wrote: > 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

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

PiotrZSL added a comment. In D141569#4139290 , @ccotter wrote: > In > > template > struct SomeClass > { > public: > explicit SomeClass(T&& value) : value(std::forward(value)) {} >T value; > }; > > `T` is not a universal reference in

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

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

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

ccotter added a comment. 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

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

ccotter added a comment. In template struct SomeClass { public: explicit SomeClass(T&& value) : value(std::forward(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

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

ccotter updated this revision to Diff 498873. ccotter added a comment. - Remove duplicated matchers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

PiotrZSL 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

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

PiotrZSL added a comment. And on something like this I still got reproduction in production code: template struct SomeClass { public: explicit SomeClass(T&& value) : value(std::forward(value)) {} T value; }; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

PiotrZSL added a comment. 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

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

PiotrZSL added a comment. 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(std::move(functor));

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

ccotter updated this revision to Diff 498730. ccotter added a comment. - Add back -fno-delayed-template-parsing to test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

PiotrZSL added a comment. In D141569#4136493 , @ccotter wrote: > @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

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

ccotter added a comment. @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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

ccotter updated this revision to Diff 498528. ccotter marked an inline comment as done. ccotter added a comment. rebase again Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter marked an inline comment as done. ccotter added a comment. Rebased as well on top of the latest release notes Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:45 +template +void

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

ccotter updated this revision to Diff 498526. ccotter added a comment. - Add back -fno-delayed-template-parsing to test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 498525. ccotter added a comment. - Add StrictMode option, fix const&& bug Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added a comment. @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

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

PiotrZSL added a comment. You right swap case can also be invalid (rvalue not needed, there). Anyway it it has too be script 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

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

carlosgalvezp added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

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

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

PiotrZSL added a comment. I run this check on project that I work for, here my comments: False-positives (for me) std::optional&& obj + std::move(opj.value()); std::pair&& obj+ std::forward>(obj); Type1&& factory + factory.create() [on initialization list] (create is not && method)

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

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. 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. Repository: rG LLVM Github

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

ccotter added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

ccotter updated this revision to Diff 494852. ccotter added a comment. - Update per https://github.com/isocpp/CppCoreGuidelines/issues/2026 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 494850. ccotter added a comment. - Add parameter name to diagnostic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:88 + + if (!Param) { +return; Nit: the style convention in LLVM is to not use braces with one-line `if/else`.

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

ccotter added a comment. 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

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

ccotter updated this revision to Diff 493004. ccotter marked an inline comment as done. ccotter added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter marked an inline comment as done. ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:1 +// RUN: %check_clang_tidy -std=c++14-or-later %s

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

ccotter updated this revision to Diff 491992. ccotter added a comment. - Use multiple runs on the same test file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:1 +// RUN: %check_clang_tidy -std=c++14-or-later %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- --

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

ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:1 +// RUN: %check_clang_tidy -std=c++14-or-later %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- --

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:1 +// RUN: %check_clang_tidy -std=c++14-or-later %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- --

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

ccotter marked an inline comment as done. ccotter added a comment. In D141569#4048359 , @njames93 wrote: > What happens with code like this > > void foo(bar&& B) { > std::move(B); > }

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

ccotter marked an inline comment as done. ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:13 + +#include + carlosgalvezp wrote: > Not sure if this is allowed in the repo or if we

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

ccotter updated this revision to Diff 491989. ccotter added a comment. - Use match instead of bespoke traversal Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 491978. ccotter added a comment. - Fix universal reference check; move diagnostic location to parameter name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:1 +// RUN: %check_clang_tidy -std=c++14-or-later %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- --

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

ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:1 +// RUN: %check_clang_tidy -std=c++14-or-later %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- --

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:13 + +#include + Not sure if this is allowed in the repo or if we should use something equivalent from ADT.

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

ccotter updated this revision to Diff 491197. ccotter added a comment. - Allow move of any expr containing the parameter Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 491186. ccotter added a comment. - Properly handle forwarding references Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 491125. ccotter added a comment. - Fix windows, fix crash Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 491124. ccotter added a comment. - Fix windows, fix crash Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added a comment. > What happens with code like this > > void foo(bar&& B) { > std::move(B); > } I raised https://github.com/isocpp/CppCoreGuidelines/issues/2026 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/

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

ccotter updated this revision to Diff 489174. ccotter added a comment. - Match unresolved calls to move Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter updated this revision to Diff 489007. ccotter added a comment. - two more tests - Minimize test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files:

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

ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:3-35 +// NOLINTBEGIN +namespace std { +template +struct remove_reference; + +template +struct remove_reference {

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

ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp:3-35 +// NOLINTBEGIN +namespace std { +template +struct remove_reference; + +template +struct remove_reference {

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

ccotter added a comment. > 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

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

njames93 added a comment. 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) {

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

ccotter updated this revision to Diff 488497. ccotter added a comment. - Does not offer fixits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new/ https://reviews.llvm.org/D141569 Files: