[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Chris Cotter via Phabricator via cfe-commits]

ccotter added a comment.

Great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In D144347#4469770 , @ccotter wrote:

> @PiotrZSL - checking back, do you plan to revisit this change soon (I think 
> there are some pending feedback and you planned some changes)? I'd like to 
> see this in change merged in. Let me know if I can help.

I will try to re-visit this change this week. I need to split this check 
somehow. Probably extract casting between types into separate bugprone check 
and add some better support for template types.
Give me 2-3 days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Chris Cotter via Phabricator via cfe-commits]

ccotter added a comment.

@PiotrZSL - checking back, do you plan to revisit this change soon (I think 
there are some pending feedback and you planned some changes)? I'd like to see 
this in change merged in. Let me know if I can help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Nathan James via Phabricator via cfe-commits]

njames93 requested changes to this revision.
njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:45
+
+inline SourceRange getAnglesLoc(const Expr *MatchedCallee) {
+  if (const auto *DeclRefExprCallee =

Use `static` instead of `inline`. Inline should only be used for functions(or 
variables) defined in header files.
Same goes for the other `inline` functions.
Also this function should be moved outside the anonymous namespace.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:47
+  if (const auto *DeclRefExprCallee =
+  llvm::dyn_cast_or_null(MatchedCallee))
+return SourceRange(DeclRefExprCallee->getLAngleLoc(),

Remove the `llvm::` qualifier and the null check is redundant here as its 
guaranteed to never be called with a non-null argument.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:52
+  if (const auto *UnresolvedLookupExprCallee =
+  llvm::dyn_cast_or_null(MatchedCallee))
+return SourceRange(UnresolvedLookupExprCallee->getLAngleLoc(),

Ditton



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:94-96
+  Finder->addMatcher(
+  traverse(
+  TK_IgnoreUnlessSpelledInSource,

Rather than calling traverse, the canoncial way to handle this is by overriding 
the `getCheckTraversalKind` virtual function to return 
`TK_IgnoreUnlessSpelledInSource`



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:99
+  unless(isExpansionInSystemHeader()), argumentCountIs(1U),
+  IgnoreDependentExpresions
+  ? expr(unless(isInstantiationDependent()))

ccotter wrote:
> I think we might need more tests when `IgnoreDependentExpresions` is true. 
> When I was playing around and hardcoded IgnoreDependentExpresions to false on 
> line 99, the tests still pass.
This needs addressing before this can be landed



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:107
+  expr(anyOf(declRefExpr(hasDeclaration(functionDecl(
+ hasName("::std::forward"))),
+ hasTemplateArgumentLoc(

It's a good idea to follow to DRY principle:
```lang=c++
auto IsNamedStdForward = hasName("::std::forward");
```
Then you can use that here and below.

A similar point can be made with the bind IDs, if you define the IDs as a 
`static constexpr char []` you can reuse them during`bind` and `getNodeAs` 
calls, helps avoid bugs if the check is ever upgraded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL planned changes to this revision.
PiotrZSL added a comment.

TODO: Extract DisableTypeMismatchSuggestion & that logic into separate check in 
bugprone category.
TODO: Validate more situations with templates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Chris Cotter via Phabricator via cfe-commits]

ccotter added inline comments.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:99
+  unless(isExpansionInSystemHeader()), argumentCountIs(1U),
+  IgnoreDependentExpresions
+  ? expr(unless(isInstantiationDependent()))

I think we might need more tests when `IgnoreDependentExpresions` is true. When 
I was playing around and hardcoded IgnoreDependentExpresions to false on line 
99, the tests still pass.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:126
+
+  Suggests removing or replacing ``std::forward`` with ``std::move`` or
+  ``static_cast`` in cases where the template argument type is invariant and

Would you see use in expanding use of this check to cases that (broadly, 
cppcoreguiideline ES.56) apply `forward` when the argument is not a forwarding 
reference? Or separate check (as the one I proposed in 
https://reviews.llvm.org/D146888)? Specifically, when the template parameter is 
deduced:

```
template  void forwards_incorrectly(T& t) { T other = 
std::forward(t); } // ignored by readability-forward-usage
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked an inline comment as done.
PiotrZSL added a comment.

I will leave this review open for 1 more week, in case someone have some 
comments, and by someone I mean you @carlosgalvezp.




Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:98
+  callExpr(
+  unless(isExpansionInSystemHeader()), argumentCountIs(1U),
+  IgnoreDependentExpresions

ccotter wrote:
> Curious, why have `isExpansionInSystemHeader` here, rather than rely on the 
> `system-headers` command line option. Is that a matter of performance (this 
> tool is not likely to apply for system headers)?
isExpansionInSystemHeader works on check level, when --system-headers on 
diagnostic level. This mean that check will be executed, and warning will be 
excluded during call to diag.
For some checks this can be expensive. 
Read more here: 
https://discourse.llvm.org/t/rfc-exclude-issues-from-system-headers-as-early-as-posible/68483


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Chris Cotter via Phabricator via cfe-commits]

ccotter accepted this revision.
ccotter added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:98
+  callExpr(
+  unless(isExpansionInSystemHeader()), argumentCountIs(1U),
+  IgnoreDependentExpresions

Curious, why have `isExpansionInSystemHeader` here, rather than rely on the 
`system-headers` command line option. Is that a matter of performance (this 
tool is not likely to apply for system headers)?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst:98
+
+.. option:: DisableTypeMismatchSuggestion
+

PiotrZSL wrote:
> ccotter wrote:
> > Curious what others thing but I think the tool should by default not remove 
> > or replace `forward` with static_cast.  When an author has written 
> > `forward`, there is a good chance they intended to forward something (e.g., 
> > they forgot to use `&&` in the parameter declaration). My hypothesis is 
> > that it's more likely the author intended to forward something, rather than 
> > than they were trying to use it as a cast (but had not intention of forward 
> > - why else would they have typed it out?). In this case, I think it merits 
> > manual review by default (so the tool should always warn, but without the 
> > fixits by default).
> No, in this case if they used forward, they used this with different type, 
> not adding & is not sufficient to trigger this case.
> 
> In theory we could, create alias bugprone-std-forward that would redirect to 
> this check, but would set DisableTypeMismatchSuggestion to false, and would 
> set DisableRemoveSuggestion & DisableMoveSuggestion to true.
> 
> In such case we could have readability check that would just suggest remove 
> of std::forward or usage of std::move, and bugprone check that would only 
> warn of casts between different types.
> 
Makes sense to me. Happy to work on that once this is merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In D144347#4222534 , @ccotter wrote:

> Is it worth adding a cppcoreguidelines alias (ES.56)?

Yes, you can pull this change (arc patch D144347 
 --nobranch) and create change that would 
depend on this one, and would register alias, also there you could override 
DisableRemoveSuggestion.
You can also help reviewing this check, to push it forward, point missing 
tests, scenarios. We could merge tests...




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst:98
+
+.. option:: DisableTypeMismatchSuggestion
+

ccotter wrote:
> Curious what others thing but I think the tool should by default not remove 
> or replace `forward` with static_cast.  When an author has written `forward`, 
> there is a good chance they intended to forward something (e.g., they forgot 
> to use `&&` in the parameter declaration). My hypothesis is that it's more 
> likely the author intended to forward something, rather than than they were 
> trying to use it as a cast (but had not intention of forward - why else would 
> they have typed it out?). In this case, I think it merits manual review by 
> default (so the tool should always warn, but without the fixits by default).
No, in this case if they used forward, they used this with different type, not 
adding & is not sufficient to trigger this case.

In theory we could, create alias bugprone-std-forward that would redirect to 
this check, but would set DisableTypeMismatchSuggestion to false, and would set 
DisableRemoveSuggestion & DisableMoveSuggestion to true.

In such case we could have readability check that would just suggest remove of 
std::forward or usage of std::move, and bugprone check that would only warn of 
casts between different types.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Chris Cotter via Phabricator via cfe-commits]

ccotter added a comment.

Is it worth adding a cppcoreguidelines alias (ES.56)?




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst:98
+
+.. option:: DisableTypeMismatchSuggestion
+

Curious what others thing but I think the tool should by default not remove or 
replace `forward` with static_cast.  When an author has written `forward`, 
there is a good chance they intended to forward something (e.g., they forgot to 
use `&&` in the parameter declaration). My hypothesis is that it's more likely 
the author intended to forward something, rather than than they were trying to 
use it as a cast (but had not intention of forward - why else would they have 
typed it out?). In this case, I think it merits manual review by default (so 
the tool should always warn, but without the fixits by default).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 503357.
PiotrZSL added a comment.

Rebase due to broken baseline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp
  clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst
  clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp
@@ -0,0 +1,257 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s readability-forward-usage %t -- -- -fno-delayed-template-parsing
+
+namespace std {
+
+struct false_type {
+  static constexpr bool value = false;
+};
+
+struct true_type {
+  static constexpr bool value = true;
+};
+
+template 
+struct is_lvalue_reference : false_type {};
+
+template 
+struct is_lvalue_reference : true_type {};
+
+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(typename std::remove_reference::type& t) noexcept {
+  return static_cast(t);
+}
+
+template 
+constexpr T&& forward(typename std::remove_reference::type&& t) noexcept {
+  static_assert(!std::is_lvalue_reference::value, "Can't forward an rvalue as an lvalue.");
+  return static_cast(t);
+}
+
+template 
+constexpr typename std::remove_reference::type&& move(T&& t) noexcept {
+  return static_cast::type&&>(t);
+}
+
+}
+
+struct TestType {
+int value = 0U;
+};
+
+struct TestTypeEx : TestType {
+};
+
+template
+void test(Args&&...) {}
+
+
+template
+void testCorrectForward(T&& value) {
+test(std::forward(value));
+}
+
+template
+void testForwardVariadicTemplate(Args&& ...args) {
+test(std::forward(args)...);
+}
+
+void callAll() {
+TestType type1;
+const TestType type2;
+testCorrectForward(type1);
+testCorrectForward(type2);
+testCorrectForward(std::move(type1));
+testCorrectForward(std::move(type2));
+testForwardVariadicTemplate(type1, TestType(), std::move(type2));
+}
+
+void testExplicit(TestType& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit2(const TestType& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit3(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit4(TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit5(const TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit6(TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: remove redundant use of 'std::forward' as it is unnecessary in this context [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(value);{{$}}
+}
+
+void testExplicit7(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: remove redundant use of 'std::forward' as it is unnecessary in this context [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(value);{{$}}
+}
+
+void testExplicit8(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended 

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 503297.
PiotrZSL added a comment.

Rebase due to broken baseline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp
  clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst
  clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp
@@ -0,0 +1,257 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s readability-forward-usage %t -- -- -fno-delayed-template-parsing
+
+namespace std {
+
+struct false_type {
+  static constexpr bool value = false;
+};
+
+struct true_type {
+  static constexpr bool value = true;
+};
+
+template 
+struct is_lvalue_reference : false_type {};
+
+template 
+struct is_lvalue_reference : true_type {};
+
+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(typename std::remove_reference::type& t) noexcept {
+  return static_cast(t);
+}
+
+template 
+constexpr T&& forward(typename std::remove_reference::type&& t) noexcept {
+  static_assert(!std::is_lvalue_reference::value, "Can't forward an rvalue as an lvalue.");
+  return static_cast(t);
+}
+
+template 
+constexpr typename std::remove_reference::type&& move(T&& t) noexcept {
+  return static_cast::type&&>(t);
+}
+
+}
+
+struct TestType {
+int value = 0U;
+};
+
+struct TestTypeEx : TestType {
+};
+
+template
+void test(Args&&...) {}
+
+
+template
+void testCorrectForward(T&& value) {
+test(std::forward(value));
+}
+
+template
+void testForwardVariadicTemplate(Args&& ...args) {
+test(std::forward(args)...);
+}
+
+void callAll() {
+TestType type1;
+const TestType type2;
+testCorrectForward(type1);
+testCorrectForward(type2);
+testCorrectForward(std::move(type1));
+testCorrectForward(std::move(type2));
+testForwardVariadicTemplate(type1, TestType(), std::move(type2));
+}
+
+void testExplicit(TestType& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit2(const TestType& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit3(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit4(TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit5(const TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit6(TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: remove redundant use of 'std::forward' as it is unnecessary in this context [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(value);{{$}}
+}
+
+void testExplicit7(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: remove redundant use of 'std::forward' as it is unnecessary in this context [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(value);{{$}}
+}
+
+void testExplicit8(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended 

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 503092.
PiotrZSL added a comment.

Ping, Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144347

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp
  clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst
  clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability/forward-usage.cpp
@@ -0,0 +1,257 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s readability-forward-usage %t -- -- -fno-delayed-template-parsing
+
+namespace std {
+
+struct false_type {
+  static constexpr bool value = false;
+};
+
+struct true_type {
+  static constexpr bool value = true;
+};
+
+template 
+struct is_lvalue_reference : false_type {};
+
+template 
+struct is_lvalue_reference : true_type {};
+
+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(typename std::remove_reference::type& t) noexcept {
+  return static_cast(t);
+}
+
+template 
+constexpr T&& forward(typename std::remove_reference::type&& t) noexcept {
+  static_assert(!std::is_lvalue_reference::value, "Can't forward an rvalue as an lvalue.");
+  return static_cast(t);
+}
+
+template 
+constexpr typename std::remove_reference::type&& move(T&& t) noexcept {
+  return static_cast::type&&>(t);
+}
+
+}
+
+struct TestType {
+int value = 0U;
+};
+
+struct TestTypeEx : TestType {
+};
+
+template
+void test(Args&&...) {}
+
+
+template
+void testCorrectForward(T&& value) {
+test(std::forward(value));
+}
+
+template
+void testForwardVariadicTemplate(Args&& ...args) {
+test(std::forward(args)...);
+}
+
+void callAll() {
+TestType type1;
+const TestType type2;
+testCorrectForward(type1);
+testCorrectForward(type2);
+testCorrectForward(std::move(type1));
+testCorrectForward(std::move(type2));
+testForwardVariadicTemplate(type1, TestType(), std::move(type2));
+}
+
+void testExplicit(TestType& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit2(const TestType& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit3(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit4(TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit5(const TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(std::move(value));{{$}}
+}
+
+void testExplicit6(TestType&& value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: remove redundant use of 'std::forward' as it is unnecessary in this context [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(value);{{$}}
+}
+
+void testExplicit7(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: remove redundant use of 'std::forward' as it is unnecessary in this context [readability-forward-usage]
+// CHECK-FIXES: {{^}}test(value);{{$}}
+}
+
+void testExplicit8(TestType value) {
+test(std::forward(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::move' instead of 'std::forward' here, as it more accurately reflects the intended purpose 

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
PiotrZSL updated this revision to Diff 498666.
PiotrZSL added a comment.
Eugene.Zelenko added reviewers: aaron.ballman, carlosgalvezp.
PiotrZSL marked 4 inline comments as done.
PiotrZSL updated this revision to Diff 498687.
PiotrZSL published this revision for review.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Fix list.rst


PiotrZSL added a comment.

Does anyone know why on WINDOWS generated AST is different:

  LINUX (when function is never called):
  |-FunctionTemplateDecl 0x5577338b2580  line:212:6 
testPartialTemplate
  | |-TemplateTypeParmDecl 0x5577338b2288  col:19 
referenced typename depth 0 index 0 T
  | `-FunctionDecl 0x5577338b24d8  line:212:6 
testPartialTemplate 'void (WrapperEx)'
  |   |-ParmVarDecl 0x5577338b23e0  col:39 referenced value 
'WrapperEx':'WrapperEx'
  |   `-CompoundStmt 0x5577338b28a0 
  | `-CallExpr 0x5577338b2878  ''
  |   |-UnresolvedLookupExpr 0x5577338b2668  '' lvalue (ADL) = 'test' 0x557733898c10
  |   `-CallExpr 0x5577338b2850  ''
  | |-UnresolvedLookupExpr 0x5577338b27b0  '' lvalue (no ADL) = 'forward' 0x5577338961d8 0x557733896818
  | `-DeclRefExpr 0x5577338b2830  'WrapperEx':'WrapperEx' 
lvalue ParmVar 0x5577338b23e0 'value' 'WrapperEx':'WrapperEx'
  
  LINUX (when function is called)
  |-FunctionTemplateDecl 0x556b33e8a5b0  line:212:6 
testPartialTemplate
  | |-TemplateTypeParmDecl 0x556b33e8a2b8  col:19 
referenced typename depth 0 index 0 T
  | |-FunctionDecl 0x556b33e8a508  line:212:6 
testPartialTemplate 'void (WrapperEx)'
  | | |-ParmVarDecl 0x556b33e8a410  col:39 referenced value 
'WrapperEx':'WrapperEx'
  | | `-CompoundStmt 0x556b33e8a8d0 
  | |   `-CallExpr 0x556b33e8a8a8  ''
  | | |-UnresolvedLookupExpr 0x556b33e8a698  '' lvalue (ADL) = 'test' 0x556b33e70c40
  | | `-CallExpr 0x556b33e8a880  ''
  | |   |-UnresolvedLookupExpr 0x556b33e8a7e0  '' lvalue (no ADL) = 'forward' 0x556b33e6e208 0x556b33e6e848
  | |   `-DeclRefExpr 0x556b33e8a860  'WrapperEx':'WrapperEx' 
lvalue ParmVar 0x556b33e8a410 'value' 'WrapperEx':'WrapperEx'
  | `-FunctionDecl 0x556b33e8e888  line:212:6 used 
testPartialTemplate 'void (WrapperEx)'
  
  WINDOWS (when function is never called):
  |-FunctionTemplateDecl 0x1ac13659de0  col:6 
testPartialTemplate
  | |-TemplateTypeParmDecl 0x1ac13652238  col:19 
referenced typename depth 0 index 0 T
  | `-FunctionDecl 0x1ac13659d38  col:6 testPartialTemplate 
'void (WrapperEx)'
  |   |-ParmVarDecl 0x1ac13652390  col:39 value 
'WrapperEx':'WrapperEx'
  |   `-<<>>
  
  WINDOWS (when function is called):
  |-FunctionTemplateDecl 0x1fe6f1b99a0  line:212:6 
testPartialTemplate
  | |-TemplateTypeParmDecl 0x1fe6f1b9568  col:19 
referenced typename depth 0 index 0 T
  | |-FunctionDecl 0x1fe6f1b98f8  line:212:6 
testPartialTemplate 'void (WrapperEx)'
  | | |-ParmVarDecl 0x1fe6f1b96c0  col:39 referenced value 
'WrapperEx':'WrapperEx'
  | | `-CompoundStmt 0x1fe6f1c5070 
  | |   `-CallExpr 0x1fe6f1c5048  ''
  | | |-UnresolvedLookupExpr 0x1fe6f1c4e40  '' lvalue (ADL) = 'test' 0x1fe6f18df70
  | | `-CallExpr 0x1fe6f1c5020  ''
  | |   |-UnresolvedLookupExpr 0x1fe6f1c4f80  '' lvalue (no ADL) = 'forward' 0x1fe6f18c3e8 0x1fe6f18ca28
  | |   `-DeclRefExpr 0x1fe6f1c5000  'WrapperEx':'WrapperEx' 
lvalue ParmVar 0x1fe6f1b96c0 'value' 'WrapperEx':'WrapperEx'
  | `-FunctionDecl 0x1fe6f1bebd8  line:212:6 used 
testPartialTemplate 'void (WrapperEx)'
  |   |-TemplateArgument type 'TestType'

EDIT: Ok, found that -fno-delayed-template-parsing is an workaround for this 
(but still strange).


PiotrZSL added a comment.

Fixed review comments and added -fno-delayed-template-parsing


PiotrZSL added a comment.

Ready for review




Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:46
+inline SourceRange getAnglesLoc(const Expr ) {
+
+  if (const auto *DeclRefExprCallee =

Excessive newline.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:55
+   UnresolvedLookupExprCallee->getRAngleLoc());
+  return SourceRange();
+}

`return {};` is shorter.



Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:123
+void ForwardUsageCheck::check(const MatchFinder::MatchResult ) {
+  const auto  = *Result.Nodes.getNodeAs("callee");
+  if (MatchedCallee.getBeginLoc().isMacroID() ||

Usually pointers are used. Same in other similar places..



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst:101
+   Disables suggestions for type mismatch situations. When this option is
+enabled, the check treats situations with different types as if they were
+cv-equal and won't suggest using ``static_cast``, but will