[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from <functional> header which are deprecated and removed in C++17

[Jens Massberg via Phabricator via cfe-commits]

massberg marked 8 inline comments as done.
massberg added a comment.

Thanks for the comments!



================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:26
+                                            .bind("base")),
+                          anyOf(isClass(), ast_matchers::isStruct()),
+                          ast_matchers::isDefinition()))
----------------
aaron.ballman wrote:
> You can drop the `ast_matchers::` here because of the using declaration above.
> 
> Actually, do we need the `isClass()` and `isStruct()` check at all? What else would match `isDerivedFrom()`?
Done.

You are right, as we use isDerivedFrom() we do not need isClass() and isStruct() anymore.


================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:33
+                     this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::mem_fun"))))
+                         .bind("mem_fun_call"),
----------------
aaron.ballman wrote:
> ::std::mem_fun
> 
> Why not `::std::mem_fun_ref`? Or the _t variants of these APIs?
There are several other types and functions in <functional> which are removed in C++17. I will add them in a follow-up change.
This change just introduces the check with a limited number of cases and helps me to get used to the clang-tidy coding style. ;)



================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+                 Result.Nodes.getNodeAs<CallExpr>("ptr_fun_call")) {
+    diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+                 Result.Nodes.getNodeAs<CallExpr>("mem_fun_call")) {
+    diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }
----------------
aaron.ballman wrote:
> I think that this code should be generalized (same with the matchers) so that you match on `hasAnyName()` for the function calls and use `CallExpr::getCalleeDecl()` to get the declaration. e.g.,
> ```
> if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("blech")) {
>   if (const Decl *Callee = Call->getCalleeDecl())
>     diag(Call->getLocStart(), Message) << Calleee;
> }
> ```
> This way you can add more named without having to add extra logic for the diagnostics.
I generalized the code and the matcher.
When we use
```
<< cast<NamedDecl>(Callee);
```
we get the template arguments in the name , e.g. `ptr_fun<int, int>`, so I chose to use `getQualifiedNameAsString`.
If there is there a better way to get the function name without template arguments I appreciate any suggestions.




https://reviews.llvm.org/D42730