[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