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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:33
+                     this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::mem_fun"))))
+                         .bind("mem_fun_call"),
----------------
massberg wrote:
> 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. ;)
> 
Okay, that's reasonable.


================
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'";
+  }
----------------
massberg wrote:
> 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.
> 
> 
Nope, in that case, your code is correct. However, we generally provide the template arguments in diagnostics. I see @alexfh was asking for them to be removed as not being useful, but I'm not certain I agree with his rationale. Yes, all instances are deprecated and thus the template arguments don't discern between good and bad cases, but showing the template arguments is also consistent with the other diagnostics we emit. For instance, other "deprecated" diagnostics also emit the template arguments. I'm not certain we should be inconsistent with the way we produce diagnostics, but I'll defer to Alex if he still feels strongly about leaving them off here.


https://reviews.llvm.org/D42730