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
