Suggests removing or replacing std::forward with std::move or
static_cast in cases where the template argument type is invariant and
the call always produces the same result.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Does anyone know why on WINDOWS generated AST is different:
LINUX (when function is never called): |-FunctionTemplateDecl 0x5577338b2580 <line:211:1, line:216:1> line:212:6 testPartialTemplate | |-TemplateTypeParmDecl 0x5577338b2288 <line:211:10, col:19> col:19 referenced typename depth 0 index 0 T | `-FunctionDecl 0x5577338b24d8 <line:212:1, line:216:1> line:212:6 testPartialTemplate 'void (WrapperEx<T>)' | |-ParmVarDecl 0x5577338b23e0 <col:26, col:39> col:39 referenced value 'WrapperEx<T>':'WrapperEx<T>' | `-CompoundStmt 0x5577338b28a0 <col:46, line:216:1> | `-CallExpr 0x5577338b2878 <line:213:5, col:41> '<dependent type>' | |-UnresolvedLookupExpr 0x5577338b2668 <col:5> '<overloaded function type>' lvalue (ADL) = 'test' 0x557733898c10 | `-CallExpr 0x5577338b2850 <col:10, col:40> '<dependent type>' | |-UnresolvedLookupExpr 0x5577338b27b0 <col:10, col:33> '<dependent type>' lvalue (no ADL) = 'forward' 0x5577338961d8 0x557733896818 | `-DeclRefExpr 0x5577338b2830 <col:35> 'WrapperEx<T>':'WrapperEx<T>' lvalue ParmVar 0x5577338b23e0 'value' 'WrapperEx<T>':'WrapperEx<T>' LINUX (when function is called) |-FunctionTemplateDecl 0x556b33e8a5b0 <line:211:1, line:216:1> line:212:6 testPartialTemplate | |-TemplateTypeParmDecl 0x556b33e8a2b8 <line:211:10, col:19> col:19 referenced typename depth 0 index 0 T | |-FunctionDecl 0x556b33e8a508 <line:212:1, line:216:1> line:212:6 testPartialTemplate 'void (WrapperEx<T>)' | | |-ParmVarDecl 0x556b33e8a410 <col:26, col:39> col:39 referenced value 'WrapperEx<T>':'WrapperEx<T>' | | `-CompoundStmt 0x556b33e8a8d0 <col:46, line:216:1> | | `-CallExpr 0x556b33e8a8a8 <line:213:5, col:41> '<dependent type>' | | |-UnresolvedLookupExpr 0x556b33e8a698 <col:5> '<overloaded function type>' lvalue (ADL) = 'test' 0x556b33e70c40 | | `-CallExpr 0x556b33e8a880 <col:10, col:40> '<dependent type>' | | |-UnresolvedLookupExpr 0x556b33e8a7e0 <col:10, col:33> '<dependent type>' lvalue (no ADL) = 'forward' 0x556b33e6e208 0x556b33e6e848 | | `-DeclRefExpr 0x556b33e8a860 <col:35> 'WrapperEx<T>':'WrapperEx<T>' lvalue ParmVar 0x556b33e8a410 'value' 'WrapperEx<T>':'WrapperEx<T>' | `-FunctionDecl 0x556b33e8e888 <line:212:1, line:216:1> line:212:6 used testPartialTemplate 'void (WrapperEx<TestType>)' WINDOWS (when function is never called): |-FunctionTemplateDecl 0x1ac13659de0 <line:211:1, line:212:44> col:6 testPartialTemplate | |-TemplateTypeParmDecl 0x1ac13652238 <line:211:10, col:19> col:19 referenced typename depth 0 index 0 T | `-FunctionDecl 0x1ac13659d38 <line:212:1, col:44> col:6 testPartialTemplate 'void (WrapperEx<T>)' | |-ParmVarDecl 0x1ac13652390 <col:26, col:39> col:39 value 'WrapperEx<T>':'WrapperEx<T>' | `-<<<NULL>>> WINDOWS (when function is called): |-FunctionTemplateDecl 0x1fe6f1b99a0 <line:211:1, line:216:1> line:212:6 testPartialTemplate | |-TemplateTypeParmDecl 0x1fe6f1b9568 <line:211:10, col:19> col:19 referenced typename depth 0 index 0 T | |-FunctionDecl 0x1fe6f1b98f8 <line:212:1, line:216:1> line:212:6 testPartialTemplate 'void (WrapperEx<T>)' | | |-ParmVarDecl 0x1fe6f1b96c0 <col:26, col:39> col:39 referenced value 'WrapperEx<T>':'WrapperEx<T>' | | `-CompoundStmt 0x1fe6f1c5070 <col:46, line:216:1> | | `-CallExpr 0x1fe6f1c5048 <line:213:5, col:41> '<dependent type>' | | |-UnresolvedLookupExpr 0x1fe6f1c4e40 <col:5> '<overloaded function type>' lvalue (ADL) = 'test' 0x1fe6f18df70 | | `-CallExpr 0x1fe6f1c5020 <col:10, col:40> '<dependent type>' | | |-UnresolvedLookupExpr 0x1fe6f1c4f80 <col:10, col:33> '<dependent type>' lvalue (no ADL) = 'forward' 0x1fe6f18c3e8 0x1fe6f18ca28 | | `-DeclRefExpr 0x1fe6f1c5000 <col:35> 'WrapperEx<T>':'WrapperEx<T>' lvalue ParmVar 0x1fe6f1b96c0 'value' 'WrapperEx<T>':'WrapperEx<T>' | `-FunctionDecl 0x1fe6f1bebd8 <line:212:1, line:216:1> line:212:6 used testPartialTemplate 'void (WrapperEx<TestType>)' | |-TemplateArgument type 'TestType'
EDIT: Ok, found that -fno-delayed-template-parsing is an workaround for this (but still strange).
Is it worth adding a cppcoreguidelines alias (ES.56)?
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst | ||
---|---|---|
99 | 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). |
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...
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst | ||
---|---|---|
99 | 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. |
clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp | ||
---|---|---|
99 | 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)? | |
clang-tools-extra/docs/clang-tidy/checks/readability/forward-usage.rst | ||
99 | Makes sense to me. Happy to work on that once this is merged. |
I will leave this review open for 1 more week, in case someone have some comments, and by someone I mean you @carlosgalvezp.
clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp | ||
---|---|---|
99 | 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. |
clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp | ||
---|---|---|
100 | 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. | |
clang-tools-extra/docs/ReleaseNotes.rst | ||
126 | 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 <class T> void forwards_incorrectly(T& t) { T other = std::forward<T>(t); } // ignored by readability-forward-usage |
TODO: Extract DisableTypeMismatchSuggestion & that logic into separate check in bugprone category.
TODO: Validate more situations with templates.
clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp | ||
---|---|---|
46 | Use static instead of inline. Inline should only be used for functions(or variables) defined in header files. | |
48 | Remove the llvm:: qualifier and the null check is redundant here as its guaranteed to never be called with a non-null argument. | |
53 | Ditton | |
95–97 | Rather than calling traverse, the canoncial way to handle this is by overriding the getCheckTraversalKind virtual function to return TK_IgnoreUnlessSpelledInSource | |
100 | This needs addressing before this can be landed | |
108 | It's a good idea to follow to DRY principle: 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. |
@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.
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.