[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
This revision was automatically updated to reflect the committed changes. Closed by commit rG5902bb9584d6: [clang-tidy] Implement cppcoreguidelines F.19 (authored by ccotter, committed by PiotrZSL). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,150 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t -- -- -fno-delayed-template-parsing + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +template +void lambda_value_capture(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=]() { T other = std::forward(t); }; +} + +template +void lambda_value_reference(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&]() { T other = std::forward(t); }; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S ot
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
PiotrZSL updated this revision to Diff 520043. PiotrZSL added a comment. Add delayed template parsing in tests (to fix windows tests) Reorder release notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,150 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t -- -- -fno-delayed-template-parsing + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +template +void lambda_value_capture(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=]() { T other = std::forward(t); }; +} + +template +void lambda_value_reference(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&]() { T other = std::forward(t); }; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
PiotrZSL added a comment. `forwarding reference parameter '' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]` consider ignoring unnamed parameters, they unused, so nothing to forward. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136 +- New :doc:`cppcoreguidelines-missing-std-forward + ` check. Should be after `cppcoreguidelines-misleading-capture-default-by-value`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 520032. ccotter added a comment. - fix merge Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,150 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +template +void lambda_value_capture(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=]() { T other = std::forward(t); }; +} + +template +void lambda_value_reference(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&]() { T other = std::forward(t); }; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) {
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter added a comment. Note, @PiotrZSL I don't have commit access, so if you're happy with the updates, could you please land this for me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 520027. ccotter added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,150 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +template +void lambda_value_capture(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=]() { T other = std::forward(t); }; +} + +template +void lambda_value_reference(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&]() { T other = std::forward(t); }; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:70 + Finder->addMatcher( + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), PiotrZSL wrote: > maybe think like: "functionDecl(forEachDescendant(parmVarDecl" could work. Not sure if I see any advantages of `forEachDescendant` over what I have here - I think I'll leave it as is. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:71 + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), + hasAncestor(functionDecl(isDefinition(), ToParam, PiotrZSL wrote: > PiotrZSL wrote: > > probably this could be named like isTemplateTypeParameter, current name > > suggest more that it's a FunctionDecl matcher, not ParmVarDecl. > consider excluding system code, or code inside std namespace. Why specifically do this in this check, but not others (very few seem to do this)? Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:72 + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), + hasAncestor(functionDecl(isDefinition(), ToParam, + unless(hasDescendant(ForwardCallMatcher), PiotrZSL wrote: > maybe we should skip also template instances. Not sure I follow. Can you give an example? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 520026. ccotter marked 6 inline comments as done. ccotter added a comment. - feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,150 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +template +void lambda_value_capture(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=]() { T other = std::forward(t); }; +} + +template +void lambda_value_reference(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&]() { T other = std::forward(t); }; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
PiotrZSL accepted this revision. PiotrZSL added a comment. This revision is now accepted and ready to land. Overall looks fine. My main concern are lambdas (and maybe functions/classes in functions, but that should only hit performance). Please close all comments before pushing this. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:61 + + StatementMatcher ForwardCallMatcher = callExpr( + argumentCountIs(1), probably `auto` would be sufficient. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:70 + Finder->addMatcher( + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), maybe think like: "functionDecl(forEachDescendant(parmVarDecl" could work. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:71 + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), + hasAncestor(functionDecl(isDefinition(), ToParam, probably this could be named like isTemplateTypeParameter, current name suggest more that it's a FunctionDecl matcher, not ParmVarDecl. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:71 + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), + hasAncestor(functionDecl(isDefinition(), ToParam, PiotrZSL wrote: > probably this could be named like isTemplateTypeParameter, current name > suggest more that it's a FunctionDecl matcher, not ParmVarDecl. consider excluding system code, or code inside std namespace. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:72 + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), + hasAncestor(functionDecl(isDefinition(), ToParam, + unless(hasDescendant(ForwardCallMatcher), maybe we should skip also template instances. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:73 + hasAncestor(functionDecl(isDefinition(), ToParam, + unless(hasDescendant(ForwardCallMatcher), + this); consider `std::move` those matchers, always check construction could be faster... Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:192 `cppcoreguidelines-macro-usage `_, + `cppcoreguidelines-missing-std-forward `_, "Yes" `cppcoreguidelines-narrowing-conversions `_, no fixes provided, remove Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp:137 +}; + +} // namespace negative_cases Add test with std::forward happen in lambda, like: ``` template void does_something(T&& t) { [=] { call_other(std::forward(t)); } } ``` this may not be detected. I usually solve those things like this: ``` auto ForwardCallMatcher = callExpr(forCallable(equalsBoundNode("first"))); ... hasAncestor(functionDecl().bind("first")), hasAncestor(functionDecl(isDefinition(), equalsBoundNode("first"), ToParam, unless(hasDescendant(ForwardCallMatcher), ``` Problem with hasAncestor, is that if first ancestor does not match then it's trying parent one. equalsBoundNode can be used to limit this only to first hasAncestor of specific type (unless there is other matcher for that). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter added a comment. bump? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 512455. ccotter marked an inline comment as done. ccotter added a comment. Fix tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t -- -- -fno-delayed-template-parsing + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + + template + AClass(U&& u) : data(std::forward(u)) {} + + template + AClass& operator=(U&& u) { +data = std::forward(u); + } + + void rvalue_ref(T&& t) { +T other = std::move(t); + } + + T data; +}; + +} // namespace negative_cases Index: clang-tools-extra/docs/clang-tidy/checks/list.rst === --- clang-tools-extra/docs/clang-tidy/checks/lis
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter marked 2 inline comments as done. ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp:138 + +} // namespace negative_cases PiotrZSL wrote: > ccotter wrote: > > ccotter wrote: > > > PiotrZSL wrote: > > > > what about when someone uses std::move instead of std::format ? > > > > maybe some "note" for such issue ? > > > Are you suggesting to have the tool add a special note in something like > > > > > > ``` > > > template > > > void foo(T&& t) { T other = std::move(t); } > > > ``` > > > > > > I'm not sure I completely followed what you were saying. Or perhaps a > > > fixit for this specific case of using move on a forwarding reference > > > (fixit to replace `move` with `forward`). > > @PiotrZSL - I wasn't sure what you meant here. > Yes, I meant that situation. But only because "forwarding reference parameter > 't' is never forwarded inside the function body" could be confused in such > case, where there is std::move instead of std::forward. > But that could be ignored, or covered by other check that would simply detect > that you doing move when you should do forward. Ya, https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html should cover this. This new check missing-std-forward only knows how to find code that is missing `forward` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
PiotrZSL added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp:138 + +} // namespace negative_cases ccotter wrote: > ccotter wrote: > > PiotrZSL wrote: > > > what about when someone uses std::move instead of std::format ? > > > maybe some "note" for such issue ? > > Are you suggesting to have the tool add a special note in something like > > > > ``` > > template > > void foo(T&& t) { T other = std::move(t); } > > ``` > > > > I'm not sure I completely followed what you were saying. Or perhaps a fixit > > for this specific case of using move on a forwarding reference (fixit to > > replace `move` with `forward`). > @PiotrZSL - I wasn't sure what you meant here. Yes, I meant that situation. But only because "forwarding reference parameter 't' is never forwarded inside the function body" could be confused in such case, where there is std::move instead of std::forward. But that could be ignored, or covered by other check that would simply detect that you doing move when you should do forward. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 512318. ccotter added a comment. - format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + + template + AClass(U&& u) : data(std::forward(u)) {} + + template + AClass& operator=(U&& u) { +data = std::forward(u); + } + + void rvalue_ref(T&& t) { +T other = std::move(t); + } + + T data; +}; + +} // namespace negative_cases Index: clang-tools-extra/docs/clang-tidy/checks/list.rst === --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-t
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp:138 + +} // namespace negative_cases ccotter wrote: > PiotrZSL wrote: > > what about when someone uses std::move instead of std::format ? > > maybe some "note" for such issue ? > Are you suggesting to have the tool add a special note in something like > > ``` > template > void foo(T&& t) { T other = std::move(t); } > ``` > > I'm not sure I completely followed what you were saying. Or perhaps a fixit > for this specific case of using move on a forwarding reference (fixit to > replace `move` with `forward`). @PiotrZSL - I wasn't sure what you meant here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 511847. ccotter added a comment. - Rename Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + + template + AClass(U&& u) : data(std::forward(u)) {} + + template + AClass& operator=(U&& u) { +data = std::forward(u); + } + + void rvalue_ref(T&& t) { +T other = std::move(t); + } + + T data; +}; + +} // namespace negative_cases Index: clang-tools-extra/docs/clang-tidy/checks/list.rst === --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-t
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 509539. ccotter added a comment. - Use common function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-forwarding-reference-param-not-forwarded %t -- -- -fno-delayed-template-parsing + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + + template + AClass(U&& u) : data(std::forward(u)) {} + + template + AClass& operator=(U&& u) {
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 508869. ccotter marked 2 inline comments as done. ccotter added a comment. - fix docs, fix test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-forwarding-reference-param-not-forwarded %t -- -- -fno-delayed-template-parsing + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + + template + AClass(U&& u) : data(std::forward(u)) {} + +
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:20 + +AST_MATCHER(Expr, hasUnevaluatedContext) { + if (isa(Node) || isa(Node)) PiotrZSL wrote: > move this matcher to some utils/... > It may be needed by other checks. > Will be done in https://reviews.llvm.org/D146929 Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp:138 + +} // namespace negative_cases PiotrZSL wrote: > what about when someone uses std::move instead of std::format ? > maybe some "note" for such issue ? Are you suggesting to have the tool add a special note in something like ``` template void foo(T&& t) { T other = std::move(t); } ``` I'm not sure I completely followed what you were saying. Or perhaps a fixit for this specific case of using move on a forwarding reference (fixit to replace `move` with `forward`). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:20 + +AST_MATCHER(Expr, hasUnevaluatedContext) { + if (isa(Node) || isa(Node)) move this matcher to some utils/... It may be needed by other checks. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h:32 + } +}; + ccotter wrote: > PiotrZSL wrote: > > add here that thing about UnlessSpelledInSource > Sorry, I think I'm missing some context here. Would you mind clarifying your > comment? I mean: ```std::optional getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; }``` just to specify this explicitly.. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst:7 +Warns when a forwarding reference parameter is not forwarded inside the +function body. + Add link to cpp guidelines, mention rule name like in other checks. Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:189 `cppcoreguidelines-avoid-reference-coroutine-parameters `_, + `cppcoreguidelines-forwarding-reference-param-not-forwarded `_, "Yes" `cppcoreguidelines-init-variables `_, "Yes" no fixes Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp:138 + +} // namespace negative_cases what about when someone uses std::move instead of std::format ? maybe some "note" for such issue ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 508450. ccotter added a comment. - add tests, simplify expr, handle unevaluated exprs - formatting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-forwarding-reference-param-not-forwarded %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + + template + AClass(U&& u) : data(std::forward(u)) {} + + template + AClass& operator=(
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:127 + + Warns when a forwarding reference parameter is not forwarded within the function body. + Please follow 80 characters limit for text. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:83-84 + + if (!Param) +return; + PiotrZSL wrote: > I thing this can never happen Another review suggested I check the matched node for nullness, even though it was the only possible match (https://reviews.llvm.org/D140793). I'll keep it as is since in the spirit of defensive programming and to assist future developers who may add more matches that might invalidate the invariant that `Param` is always non-null. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h:32 + } +}; + PiotrZSL wrote: > add here that thing about UnlessSpelledInSource Sorry, I think I'm missing some context here. Would you mind clarifying your comment? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
ccotter updated this revision to Diff 508441. ccotter marked 4 inline comments as done. ccotter retitled this revision from "[clang-tidy] Implement cppcoreguidelines F.19 " to "[clang-tidy] Implement cppcoreguidelines F.19". ccotter added a comment. - add tests, simplify expr, handle unevaluated exprs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp === --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-forwarding-reference-param-not-forwarded %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + + template + void mixed_params(T&& t, U&& u) { +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] +T other1 = std::move(t); +U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_refer
[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19
PiotrZSL added a comment. maybe some other name for check, like missing-std-forward. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:62 + namedDecl(hasUnderlyingDecl(hasName("::std::forward")), + argumentCountIs(1), + hasArgument(0, declRefExpr(to(equalsBoundNode("param"))).bind("ref"))) move this up to be checkered first Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:67 + Finder->addMatcher( + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), anyway invert this logic. would be nice to invert this to start matching with functionDecl, but we don't have forEachParameter Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:69-72 + anyOf(hasAncestor(cxxConstructorDecl( +ToParam, isDefinition(), +optionally(hasDescendant(ForwardCallMatcher, +hasAncestor(functionDecl( use isDefinition also for functionDecl, anyway maybe ``` hasAncestor(functionnDecl(isDefinition(), ToParam, unless(hasDescendant(ForwardCallMatcher))) ``` I don't see reason for eplicit handling of cxxConstructorDecl, hasDescendant will match also initialization list. Exclude code that is used in decltype, like: ``` template void test(T&& t) { using Type = decltype(callSomething(std::forward(t))); callOther(t); } ``` Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp:83-84 + + if (!Param) +return; + I thing this can never happen Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h:32 + } +}; + add here that thing about UnlessSpelledInSource Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146921/new/ https://reviews.llvm.org/D146921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits