[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > aaron.ballman wrote:
> > > > > > alexfh wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > alexfh wrote:
> > > > > > > > > alexfh wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > 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("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(Callee);
> > > > > > > > > > > > ```
> > > > > > > > > > > > we get the template arguments in the name , e.g. 
> > > > > > > > > > > > `ptr_fun`, 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.
> > > > > > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > > > > > arguments. Moreover, they seem to be issued only on 
> > > > > > > > > > instantiations, see https://godbolt.org/g/W563gw.
> > > > > > > > > > 
> > > > > > > > > > But I have a number of concerns with template arguments in 
> > > > > > > > > > the deprecation warnings:
> > > > > > > > > > 
> > > > > > > > > > 1. The note attached to the warning lies. Consider a 
> > > > > > > > > > warning from the test above:
> > > > > > > > > >   ...
> > > > > > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > > > > > [-Wdeprecated-declarations]
> > > > > > > > > >   B e;
> > > > > > > > > >   ^
> > > > > > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > > > > > deprecated here
> > > > > > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > > > > > 
> > > > > > > > > >  But `B` hasn't been explicitly marked deprecated, 
> > > > > > > > > > only the template definition of `B` has been. Template 
> > > > > > > > > > arguments are important in the case of the explicit 
> > > > > > > > > > template specialization `A` in the same example, but 
> > > > > > > > > > not in cases where the template definition was marked 
> > > > > > > > > > deprecated, since template arguments only add noise and no 
> > > > > > > > > > useful information there.
> > > > > > > > > > 2. Warnings can easily become unreadable: 
> > > > > > > > > > https://godbolt.org/g/AFdznH
> > > > > > > > > > 3. Certain coding patterns can result in numerous 
> > > > > > > > > > deprecation warnings differing only in template arguments: 
> > > > > > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate 
> > > > > > > > > > warnings, if they have 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > alexfh wrote:
> > > > > > > > alexfh wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > 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("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(Callee);
> > > > > > > > > > > ```
> > > > > > > > > > > we get the template arguments in the name , e.g. 
> > > > > > > > > > > `ptr_fun`, 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.
> > > > > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > > > > arguments. Moreover, they seem to be issued only on 
> > > > > > > > > instantiations, see https://godbolt.org/g/W563gw.
> > > > > > > > > 
> > > > > > > > > But I have a number of concerns with template arguments in 
> > > > > > > > > the deprecation warnings:
> > > > > > > > > 
> > > > > > > > > 1. The note attached to the warning lies. Consider a warning 
> > > > > > > > > from the test above:
> > > > > > > > >   ...
> > > > > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > > > > [-Wdeprecated-declarations]
> > > > > > > > >   B e;
> > > > > > > > >   ^
> > > > > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > > > > deprecated here
> > > > > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > > > > 
> > > > > > > > >  But `B` hasn't been explicitly marked deprecated, only 
> > > > > > > > > the template definition of `B` has been. Template arguments 
> > > > > > > > > are important in the case of the explicit template 
> > > > > > > > > specialization `A` in the same example, but not in cases 
> > > > > > > > > where the template definition was marked deprecated, since 
> > > > > > > > > template arguments only add noise and no useful information 
> > > > > > > > > there.
> > > > > > > > > 2. Warnings can easily become unreadable: 
> > > > > > > > > https://godbolt.org/g/AFdznH
> > > > > > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > > > > > warnings differing only in template arguments: 
> > > > > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate 
> > > > > > > > > warnings, if they have identical text and location, but 
> > > > > > > > > adding template arguments to the message will prevent 
> > > > > > > > > deduplication. I've seen a case where thousands of 
> > > > > > > > > deprecation warnings were generated for a 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > aaron.ballman wrote:
> > > > > > alexfh wrote:
> > > > > > > alexfh wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > 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("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(Callee);
> > > > > > > > > > ```
> > > > > > > > > > we get the template arguments in the name , e.g. 
> > > > > > > > > > `ptr_fun`, 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.
> > > > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > > > arguments. Moreover, they seem to be issued only on 
> > > > > > > > instantiations, see https://godbolt.org/g/W563gw.
> > > > > > > > 
> > > > > > > > But I have a number of concerns with template arguments in the 
> > > > > > > > deprecation warnings:
> > > > > > > > 
> > > > > > > > 1. The note attached to the warning lies. Consider a warning 
> > > > > > > > from the test above:
> > > > > > > >   ...
> > > > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > > > [-Wdeprecated-declarations]
> > > > > > > >   B e;
> > > > > > > >   ^
> > > > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > > > deprecated here
> > > > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > > > 
> > > > > > > >  But `B` hasn't been explicitly marked deprecated, only 
> > > > > > > > the template definition of `B` has been. Template arguments are 
> > > > > > > > important in the case of the explicit template specialization 
> > > > > > > > `A` in the same example, but not in cases where the 
> > > > > > > > template definition was marked deprecated, since template 
> > > > > > > > arguments only add noise and no useful information there.
> > > > > > > > 2. Warnings can easily become unreadable: 
> > > > > > > > https://godbolt.org/g/AFdznH
> > > > > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > > > > warnings differing only in template arguments: 
> > > > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate 
> > > > > > > > warnings, if they have identical text and location, but adding 
> > > > > > > > template arguments to the message will prevent deduplication. 
> > > > > > > > I've seen a case where thousands of deprecation warnings were 
> > > > > > > > generated for a single line in a widely used header.
> > > > > > > > 
> > > > > > > > So yes, I feel strongly about leaving off template arguments in 
> > > > > > > > case the whole template was marked 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > alexfh wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 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("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(Callee);
> > > > > > > > > ```
> > > > > > > > > we get the template arguments in the name , e.g. 
> > > > > > > > > `ptr_fun`, 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.
> > > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > > arguments. Moreover, they seem to be issued only on 
> > > > > > > instantiations, see https://godbolt.org/g/W563gw.
> > > > > > > 
> > > > > > > But I have a number of concerns with template arguments in the 
> > > > > > > deprecation warnings:
> > > > > > > 
> > > > > > > 1. The note attached to the warning lies. Consider a warning from 
> > > > > > > the test above:
> > > > > > >   ...
> > > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > > [-Wdeprecated-declarations]
> > > > > > >   B e;
> > > > > > >   ^
> > > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > > deprecated here
> > > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > > 
> > > > > > >  But `B` hasn't been explicitly marked deprecated, only the 
> > > > > > > template definition of `B` has been. Template arguments are 
> > > > > > > important in the case of the explicit template specialization 
> > > > > > > `A` in the same example, but not in cases where the template 
> > > > > > > definition was marked deprecated, since template arguments only 
> > > > > > > add noise and no useful information there.
> > > > > > > 2. Warnings can easily become unreadable: 
> > > > > > > https://godbolt.org/g/AFdznH
> > > > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > > > warnings differing only in template arguments: 
> > > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate 
> > > > > > > warnings, if they have identical text and location, but adding 
> > > > > > > template arguments to the message will prevent deduplication. 
> > > > > > > I've seen a case where thousands of deprecation warnings were 
> > > > > > > generated for a single line in a widely used header.
> > > > > > > 
> > > > > > > So yes, I feel strongly about leaving off template arguments in 
> > > > > > > case the whole template was marked deprecated. I think it would 
> > > > > > > be the right thing to do for the -Wdeprecated-declarations 
> > > > > > > diagnostic as well.
> > > > > > s/leaving off/leaving out/
> > > > > > The note attached to the 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > alexfh wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 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("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(Callee);
> > > > > > > > ```
> > > > > > > > we get the template arguments in the name , e.g. `ptr_fun > > > > > > > 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.
> > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > arguments. Moreover, they seem to be issued only on instantiations, 
> > > > > > see https://godbolt.org/g/W563gw.
> > > > > > 
> > > > > > But I have a number of concerns with template arguments in the 
> > > > > > deprecation warnings:
> > > > > > 
> > > > > > 1. The note attached to the warning lies. Consider a warning from 
> > > > > > the test above:
> > > > > >   ...
> > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > [-Wdeprecated-declarations]
> > > > > >   B e;
> > > > > >   ^
> > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > deprecated here
> > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > 
> > > > > >  But `B` hasn't been explicitly marked deprecated, only the 
> > > > > > template definition of `B` has been. Template arguments are 
> > > > > > important in the case of the explicit template specialization 
> > > > > > `A` in the same example, but not in cases where the template 
> > > > > > definition was marked deprecated, since template arguments only add 
> > > > > > noise and no useful information there.
> > > > > > 2. Warnings can easily become unreadable: 
> > > > > > https://godbolt.org/g/AFdznH
> > > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > > warnings differing only in template arguments: 
> > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate warnings, 
> > > > > > if they have identical text and location, but adding template 
> > > > > > arguments to the message will prevent deduplication. I've seen a 
> > > > > > case where thousands of deprecation warnings were generated for a 
> > > > > > single line in a widely used header.
> > > > > > 
> > > > > > So yes, I feel strongly about leaving off template arguments in 
> > > > > > case the whole template was marked deprecated. I think it would be 
> > > > > > the right thing to do for the -Wdeprecated-declarations diagnostic 
> > > > > > as well.
> > > > > s/leaving off/leaving out/
> > > > > The note attached to the warning lies.
> > > > 
> > > > No it doesn't? The attribute is inherited from the primary template 
> > > > unless your explicit specialization *removes* the attribute. 
> > > > 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > alexfh wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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("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(Callee);
> > > > > > > ```
> > > > > > > we get the template arguments in the name , e.g. `ptr_fun > > > > > > 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.
> > > > > Indeed, -Wdeprecated-declarations warnings print template arguments. 
> > > > > Moreover, they seem to be issued only on instantiations, see 
> > > > > https://godbolt.org/g/W563gw.
> > > > > 
> > > > > But I have a number of concerns with template arguments in the 
> > > > > deprecation warnings:
> > > > > 
> > > > > 1. The note attached to the warning lies. Consider a warning from the 
> > > > > test above:
> > > > >   ...
> > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > [-Wdeprecated-declarations]
> > > > >   B e;
> > > > >   ^
> > > > >   :7:10: note: 'B' has been explicitly marked deprecated 
> > > > > here
> > > > >   struct [[deprecated("bbb")]] B {};
> > > > > 
> > > > >  But `B` hasn't been explicitly marked deprecated, only the 
> > > > > template definition of `B` has been. Template arguments are important 
> > > > > in the case of the explicit template specialization `A` in the 
> > > > > same example, but not in cases where the template definition was 
> > > > > marked deprecated, since template arguments only add noise and no 
> > > > > useful information there.
> > > > > 2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
> > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > warnings differing only in template arguments: 
> > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate warnings, if 
> > > > > they have identical text and location, but adding template arguments 
> > > > > to the message will prevent deduplication. I've seen a case where 
> > > > > thousands of deprecation warnings were generated for a single line in 
> > > > > a widely used header.
> > > > > 
> > > > > So yes, I feel strongly about leaving off template arguments in case 
> > > > > the whole template was marked deprecated. I think it would be the 
> > > > > right thing to do for the -Wdeprecated-declarations diagnostic as 
> > > > > well.
> > > > s/leaving off/leaving out/
> > > > The note attached to the warning lies.
> > > 
> > > No it doesn't? The attribute is inherited from the primary template 
> > > unless your explicit specialization *removes* the attribute. 
> > > https://godbolt.org/g/ZuXZds
> > > 
> > > > Warnings can easily become unreadable
> > > 
> > > This is true of all template diagnostics and isn't specific to 
> > > clang-tidy's treatment of them.
> > > 
> > > > I've seen a case 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > 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("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(Callee);
> > > > > > ```
> > > > > > we get the template arguments in the name , e.g. `ptr_fun > > > > > 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.
> > > > Indeed, -Wdeprecated-declarations warnings print template arguments. 
> > > > Moreover, they seem to be issued only on instantiations, see 
> > > > https://godbolt.org/g/W563gw.
> > > > 
> > > > But I have a number of concerns with template arguments in the 
> > > > deprecation warnings:
> > > > 
> > > > 1. The note attached to the warning lies. Consider a warning from the 
> > > > test above:
> > > >   ...
> > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > [-Wdeprecated-declarations]
> > > >   B e;
> > > >   ^
> > > >   :7:10: note: 'B' has been explicitly marked deprecated 
> > > > here
> > > >   struct [[deprecated("bbb")]] B {};
> > > > 
> > > >  But `B` hasn't been explicitly marked deprecated, only the 
> > > > template definition of `B` has been. Template arguments are important 
> > > > in the case of the explicit template specialization `A` in the 
> > > > same example, but not in cases where the template definition was marked 
> > > > deprecated, since template arguments only add noise and no useful 
> > > > information there.
> > > > 2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
> > > > 3. Certain coding patterns can result in numerous deprecation warnings 
> > > > differing only in template arguments: https://godbolt.org/g/U2JCbG. 
> > > > Clang-tidy can deduplicate warnings, if they have identical text and 
> > > > location, but adding template arguments to the message will prevent 
> > > > deduplication. I've seen a case where thousands of deprecation warnings 
> > > > were generated for a single line in a widely used header.
> > > > 
> > > > So yes, I feel strongly about leaving off template arguments in case 
> > > > the whole template was marked deprecated. I think it would be the right 
> > > > thing to do for the -Wdeprecated-declarations diagnostic as well.
> > > s/leaving off/leaving out/
> > > The note attached to the warning lies.
> > 
> > No it doesn't? The attribute is inherited from the primary template unless 
> > your explicit specialization *removes* the attribute. 
> > https://godbolt.org/g/ZuXZds
> > 
> > > Warnings can easily become unreadable
> > 
> > This is true of all template diagnostics and isn't specific to clang-tidy's 
> > treatment of them.
> > 
> > > I've seen a case where thousands of deprecation warnings were generated 
> > > for a single line in a widely used header.
> > 
> > This sounds more worrying, but at the same time, your link behaving by 
> > design and doing what I would want it 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

aaron.ballman wrote:
> alexfh wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > 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("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(Callee);
> > > > > ```
> > > > > we get the template arguments in the name , e.g. `ptr_fun`, 
> > > > > 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.
> > > Indeed, -Wdeprecated-declarations warnings print template arguments. 
> > > Moreover, they seem to be issued only on instantiations, see 
> > > https://godbolt.org/g/W563gw.
> > > 
> > > But I have a number of concerns with template arguments in the 
> > > deprecation warnings:
> > > 
> > > 1. The note attached to the warning lies. Consider a warning from the 
> > > test above:
> > >   ...
> > >   :11:1: warning: 'B' is deprecated: bbb 
> > > [-Wdeprecated-declarations]
> > >   B e;
> > >   ^
> > >   :7:10: note: 'B' has been explicitly marked deprecated here
> > >   struct [[deprecated("bbb")]] B {};
> > > 
> > >  But `B` hasn't been explicitly marked deprecated, only the template 
> > > definition of `B` has been. Template arguments are important in the case 
> > > of the explicit template specialization `A` in the same example, but 
> > > not in cases where the template definition was marked deprecated, since 
> > > template arguments only add noise and no useful information there.
> > > 2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
> > > 3. Certain coding patterns can result in numerous deprecation warnings 
> > > differing only in template arguments: https://godbolt.org/g/U2JCbG. 
> > > Clang-tidy can deduplicate warnings, if they have identical text and 
> > > location, but adding template arguments to the message will prevent 
> > > deduplication. I've seen a case where thousands of deprecation warnings 
> > > were generated for a single line in a widely used header.
> > > 
> > > So yes, I feel strongly about leaving off template arguments in case the 
> > > whole template was marked deprecated. I think it would be the right thing 
> > > to do for the -Wdeprecated-declarations diagnostic as well.
> > s/leaving off/leaving out/
> > The note attached to the warning lies.
> 
> No it doesn't? The attribute is inherited from the primary template unless 
> your explicit specialization *removes* the attribute. 
> https://godbolt.org/g/ZuXZds
> 
> > Warnings can easily become unreadable
> 
> This is true of all template diagnostics and isn't specific to clang-tidy's 
> treatment of them.
> 
> > I've seen a case where thousands of deprecation warnings were generated for 
> > a single line in a widely used header.
> 
> This sounds more worrying, but at the same time, your link behaving by design 
> and doing what I would want it to do. The presence of the deprecated primary 
> template isn't what gets diagnosed, it's the *uses* of the deprecated entity. 
> This is called out explicitly in [dcl.attr.deprecated]p4.
> 
> > So yes, I feel strongly about 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > 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("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(Callee);
> > > > ```
> > > > we get the template arguments in the name , e.g. `ptr_fun`, 
> > > > 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.
> > Indeed, -Wdeprecated-declarations warnings print template arguments. 
> > Moreover, they seem to be issued only on instantiations, see 
> > https://godbolt.org/g/W563gw.
> > 
> > But I have a number of concerns with template arguments in the deprecation 
> > warnings:
> > 
> > 1. The note attached to the warning lies. Consider a warning from the test 
> > above:
> >   ...
> >   :11:1: warning: 'B' is deprecated: bbb 
> > [-Wdeprecated-declarations]
> >   B e;
> >   ^
> >   :7:10: note: 'B' has been explicitly marked deprecated here
> >   struct [[deprecated("bbb")]] B {};
> > 
> >  But `B` hasn't been explicitly marked deprecated, only the template 
> > definition of `B` has been. Template arguments are important in the case of 
> > the explicit template specialization `A` in the same example, but not 
> > in cases where the template definition was marked deprecated, since 
> > template arguments only add noise and no useful information there.
> > 2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
> > 3. Certain coding patterns can result in numerous deprecation warnings 
> > differing only in template arguments: https://godbolt.org/g/U2JCbG. 
> > Clang-tidy can deduplicate warnings, if they have identical text and 
> > location, but adding template arguments to the message will prevent 
> > deduplication. I've seen a case where thousands of deprecation warnings 
> > were generated for a single line in a widely used header.
> > 
> > So yes, I feel strongly about leaving off template arguments in case the 
> > whole template was marked deprecated. I think it would be the right thing 
> > to do for the -Wdeprecated-declarations diagnostic as well.
> s/leaving off/leaving out/
> The note attached to the warning lies.

No it doesn't? The attribute is inherited from the primary template unless your 
explicit specialization *removes* the attribute. https://godbolt.org/g/ZuXZds

> Warnings can easily become unreadable

This is true of all template diagnostics and isn't specific to clang-tidy's 
treatment of them.

> I've seen a case where thousands of deprecation warnings were generated for a 
> single line in a widely used header.

This sounds more worrying, but at the same time, your link behaving by design 
and doing what I would want it to do. The presence of the deprecated primary 
template isn't what gets diagnosed, it's the *uses* of the deprecated entity. 
This is called out explicitly in [dcl.attr.deprecated]p4.

> So yes, I feel strongly about leaving off template arguments in case the 
> whole template was marked deprecated. I think it would be the right thing to 
> do for the -Wdeprecated-declarations diagnostic as well.

I would be strongly opposed to 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> aaron.ballman wrote:
> > 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("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(Callee);
> > > ```
> > > we get the template arguments in the name , e.g. `ptr_fun`, 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.
> Indeed, -Wdeprecated-declarations warnings print template arguments. 
> Moreover, they seem to be issued only on instantiations, see 
> https://godbolt.org/g/W563gw.
> 
> But I have a number of concerns with template arguments in the deprecation 
> warnings:
> 
> 1. The note attached to the warning lies. Consider a warning from the test 
> above:
>   ...
>   :11:1: warning: 'B' is deprecated: bbb 
> [-Wdeprecated-declarations]
>   B e;
>   ^
>   :7:10: note: 'B' has been explicitly marked deprecated here
>   struct [[deprecated("bbb")]] B {};
> 
>  But `B` hasn't been explicitly marked deprecated, only the template 
> definition of `B` has been. Template arguments are important in the case of 
> the explicit template specialization `A` in the same example, but not in 
> cases where the template definition was marked deprecated, since template 
> arguments only add noise and no useful information there.
> 2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
> 3. Certain coding patterns can result in numerous deprecation warnings 
> differing only in template arguments: https://godbolt.org/g/U2JCbG. 
> Clang-tidy can deduplicate warnings, if they have identical text and 
> location, but adding template arguments to the message will prevent 
> deduplication. I've seen a case where thousands of deprecation warnings were 
> generated for a single line in a widely used header.
> 
> So yes, I feel strongly about leaving off template arguments in case the 
> whole template was marked deprecated. I think it would be the right thing to 
> do for the -Wdeprecated-declarations diagnostic as well.
s/leaving off/leaving out/


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

aaron.ballman wrote:
> 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("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(Callee);
> > ```
> > we get the template arguments in the name , e.g. `ptr_fun`, 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.
Indeed, -Wdeprecated-declarations warnings print template arguments. Moreover, 
they seem to be issued only on instantiations, see https://godbolt.org/g/W563gw.

But I have a number of concerns with template arguments in the deprecation 
warnings:

1. The note attached to the warning lies. Consider a warning from the test 
above:
  ...
  :11:1: warning: 'B' is deprecated: bbb 
[-Wdeprecated-declarations]
  B e;
  ^
  :7:10: note: 'B' has been explicitly marked deprecated here
  struct [[deprecated("bbb")]] B {};

 But `B` hasn't been explicitly marked deprecated, only the template 
definition of `B` has been. Template arguments are important in the case of the 
explicit template specialization `A` in the same example, but not in cases 
where the template definition was marked deprecated, since template arguments 
only add noise and no useful information there.
2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
3. Certain coding patterns can result in numerous deprecation warnings 
differing only in template arguments: https://godbolt.org/g/U2JCbG. Clang-tidy 
can deduplicate warnings, if they have identical text and location, but adding 
template arguments to the message will prevent deduplication. I've seen a case 
where thousands of deprecation warnings were generated for a single line in a 
widely used header.

So yes, I feel strongly about leaving off template arguments in case the whole 
template was marked deprecated. I think it would be the right thing to do for 
the -Wdeprecated-declarations diagnostic as well.


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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  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("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("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("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(Callee);
> ```
> we get the template arguments in the name , e.g. `ptr_fun`, 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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  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("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("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("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(Callee);
```
we get the template arguments in the name , e.g. `ptr_fun`, 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jens Massberg via Phabricator via cfe-commits]

massberg updated this revision to Diff 134578.
massberg edited the summary of this revision.

https://reviews.llvm.org/D42730

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/DeprecatedFunctionalCheck.cpp
  clang-tidy/modernize/DeprecatedFunctionalCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-deprecated-functional.rst
  test/clang-tidy/modernize-deprecated-functional.cpp

Index: test/clang-tidy/modernize-deprecated-functional.cpp
===
--- test/clang-tidy/modernize-deprecated-functional.cpp
+++ test/clang-tidy/modernize-deprecated-functional.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s modernize-deprecated-functional %t
+
+namespace std {
+
+template 
+class binary_function {};
+
+template 
+class unary_function {};
+
+template 
+class pointer_to_unary_function
+: public std::unary_function {  // NOLINT
+};
+
+template 
+pointer_to_unary_function ptr_fun(Result (*f)(Arg)) {  // NOLINT
+  pointer_to_unary_function Nothing;
+  return Nothing;
+}
+
+template 
+class mem_fun_t {};
+
+template 
+mem_fun_t mem_fun(Res (T::*f)()) {}  // NOLINT
+
+}  // namespace std
+
+// CHECK-MESSAGES: [[@LINE+1]]:25: warning: 'binary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class BinaryTestClass : public std::binary_function {
+ public:
+  BinaryTestClass();
+};
+
+// CHECK-MESSAGES: [[@LINE+1]]:24: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class UnaryTestClass : public std::unary_function {
+ public:
+  UnaryTestClass();
+};
+
+class TestClass {
+ public:
+  int test() { return 1; }
+};
+
+// CHECK-MESSAGES: [[@LINE+2]]:31: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class AnotherUnaryTestClass : public TestClass,
+  public std::unary_function {
+ public:
+  AnotherUnaryTestClass();
+};
+
+
+int simpleFunc(int X) { return X + 1; };
+
+void foo(void) {
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::ptr_fun' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+  std::ptr_fun(simpleFunc);
+
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::mem_fun' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+  std::mem_fun(::test);
+}
Index: docs/clang-tidy/checks/modernize-deprecated-functional.rst
===
--- docs/clang-tidy/checks/modernize-deprecated-functional.rst
+++ docs/clang-tidy/checks/modernize-deprecated-functional.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - modernize-deprecated-functional
+
+modernize-deprecated-functional
+===
+
+Warns if types, classes, and functions from the `` header which are deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is
+used:
+
+-  `std::unary_function`
+-  `std::binary_function`
+-  `std::ptr_fun`
+-  `std::mem_fun`
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -155,6 +155,7 @@
misc-unused-raii
misc-unused-using-decls
modernize-avoid-bind
+   modernize-deprecated-functional
modernize-deprecated-headers
modernize-loop-convert
modernize-make-shared
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -95,6 +95,12 @@
   Warns if SIMD intrinsics are used which can be replaced by
   ``std::experimental::simd`` operations.
 
+- New `modernize-deprecated-functional
+  `_ check
+
+  Warns if types, classes, and functions from the `` header which
+  are deprecated in C++11 and removed in C++17 are used.
+
 - New alias `hicpp-avoid-goto
   `_ to 
   `cppcoreguidelines-avoid-goto `_
Index: clang-tidy/modernize/ModernizeTidyModule.cpp
===
--- clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidBindCheck.h"
+#include "DeprecatedFunctionalCheck.h"
 #include "DeprecatedHeadersCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
@@ -45,6 +46,8 @@
 public:
   void addCheckFactories(ClangTidyCheckFactories ) 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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:26
+.bind("base")),
+  anyOf(isClass(), ast_matchers::isStruct()),
+  ast_matchers::isDefinition()))

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()`?



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:30
+  this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::ptr_fun"
+ .bind("ptr_fun_call"),

::std::ptr_fun



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:33
+ this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::mem_fun"
+ .bind("mem_fun_call"),

::std::mem_fun

Why not `::std::mem_fun_ref`? Or the _t variants of these APIs?



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

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("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.



Comment at: docs/ReleaseNotes.rst:95
+
+  Warns if types, classes, and functions from '' header which are
+  deprecated in C++11 and removed in C++17 are used.

Please use backticks for ``. Also, should say "and functions from 
the  header".



Comment at: docs/clang-tidy/checks/modernize-deprecated-functional.rst:6
+
+Warns if types, classes, and functions from '' header which are 
deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is

Same comments here as above.


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jens Massberg via Phabricator via cfe-commits]

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

Thanks for the comments!
I double-checked that the renaming went well and hope that I haven't missed 
anything this time ...


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jens Massberg via Phabricator via cfe-commits]

massberg updated this revision to Diff 133027.
massberg edited the summary of this revision.
massberg added a comment.

Addressed comments and renamed test to modernize-deprecated-functional


https://reviews.llvm.org/D42730

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/DeprecatedFunctionalCheck.cpp
  clang-tidy/modernize/DeprecatedFunctionalCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-deprecated-functional.rst
  test/clang-tidy/modernize-deprecated-functional.cpp

Index: test/clang-tidy/modernize-deprecated-functional.cpp
===
--- test/clang-tidy/modernize-deprecated-functional.cpp
+++ test/clang-tidy/modernize-deprecated-functional.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s modernize-deprecated-functional %t
+
+namespace std {
+
+template 
+class binary_function {};
+
+template 
+class unary_function {};
+
+template 
+class pointer_to_unary_function
+: public std::unary_function {  // NOLINT
+};
+
+template 
+pointer_to_unary_function ptr_fun(Result (*f)(Arg)) {
+  pointer_to_unary_function Nothing;
+  return Nothing;
+}
+
+template 
+class mem_fun_t {};
+
+template 
+mem_fun_t mem_fun(Res (T::*f)()) {}
+
+}  // namespace std
+
+// CHECK-MESSAGES: [[@LINE+1]]:25: warning: 'binary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class BinaryTestClass : public std::binary_function {
+ public:
+  BinaryTestClass();
+};
+
+// CHECK-MESSAGES: [[@LINE+1]]:24: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class UnaryTestClass : public std::unary_function {
+ public:
+  UnaryTestClass();
+};
+
+class TestClass {
+ public:
+  int test() { return 1; }
+};
+
+// CHECK-MESSAGES: [[@LINE+2]]:31: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class AnotherUnaryTestClass : public TestClass,
+  public std::unary_function {
+ public:
+  AnotherUnaryTestClass();
+};
+
+
+int simpleFunc(int X) { return X + 1; };
+
+void foo(void) {
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::ptr_fun' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+  std::ptr_fun(simpleFunc);
+
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::mem_fun' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+  std::mem_fun(::test);
+}
Index: docs/clang-tidy/checks/modernize-deprecated-functional.rst
===
--- docs/clang-tidy/checks/modernize-deprecated-functional.rst
+++ docs/clang-tidy/checks/modernize-deprecated-functional.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - modernize-deprecated-functional
+
+modernize-deprecated-functional
+===
+
+Warns if types, classes, and functions from '' header which are deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is
+used:
+
+-  `std::unary_function`
+-  `std::binary_function`
+-  `std::ptr_fun`
+-  `std::mem_fun`
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -154,6 +154,7 @@
misc-unused-raii
misc-unused-using-decls
modernize-avoid-bind
+   modernize-deprecated-functional
modernize-deprecated-headers
modernize-loop-convert
modernize-make-shared
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -89,6 +89,12 @@
   using decltype specifiers and lambda with otherwise unutterable 
   return types.
 
+- New `modernize-deprecated-functional
+  `_ check
+
+  Warns if types, classes, and functions from '' header which are
+  deprecated in C++11 and removed in C++17 are used.
+
 - New alias `hicpp-avoid-goto
   `_ to 
   `cppcoreguidelines-avoid-goto `_
Index: clang-tidy/modernize/ModernizeTidyModule.cpp
===
--- clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidBindCheck.h"
+#include "DeprecatedFunctionalCheck.h"
 #include "DeprecatedHeadersCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
@@ -45,6 +46,8 @@
 public:
   

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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/ModernizeTidyModule.cpp:50
+CheckFactories.registerCheck(
+"modernize-avoid-functional");
 CheckFactories.registerCheck(

alexfh wrote:
> aaron.ballman wrote:
> > I'm not keen on this name -- it suggests the user should avoid functional, 
> > which is certainly not the advice we want to give them. Also, it makes no 
> > mention of deprecations, which is what the check is all about.
> > 
> > How about: `modernize-functional-deprecations` and we can use 
> > `modernize-deprecations` as the eventual catch-all umbrella, 
> > `modernize--deprecations` for other headers, and 
> > `modernize--deprecations-` if we want to add API-level 
> > granularity?
> I'd suggest to put "deprecations" first: modernize-deprecations-functional. 
> Or maybe modernize-deprecated-functional?
That works for me -- I have a slight preference for 
`modernize-deprecated-functional`.


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:50
+CheckFactories.registerCheck(
+"modernize-avoid-functional");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> I'm not keen on this name -- it suggests the user should avoid functional, 
> which is certainly not the advice we want to give them. Also, it makes no 
> mention of deprecations, which is what the check is all about.
> 
> How about: `modernize-functional-deprecations` and we can use 
> `modernize-deprecations` as the eventual catch-all umbrella, 
> `modernize--deprecations` for other headers, and 
> `modernize--deprecations-` if we want to add API-level 
> granularity?
I'd suggest to put "deprecations" first: modernize-deprecations-functional. Or 
maybe modernize-deprecated-functional?


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

A few comments were not applied, and I'd like a more descriptive name for the 
check (especially if we plan to generalize this).




Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > aaron.ballman wrote:
> > > > > > alexfh wrote:
> > > > > > > Quuxplusone wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > alexfh wrote:
> > > > > > > > > > alexfh wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > > > > > > ``? Just like we have a "deprecated 
> > > > > > > > > > > > > > > headers" check, perhaps this should be a 
> > > > > > > > > > > > > > > "deprecated APIs" check?
> > > > > > > > > > > > > > Added full stop.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > > > > >  or be extended to a full 'deprecated 
> > > > > > > > > > > > > > API' check.
> > > > > > > > > > > > > > This change is just a start, several more types and 
> > > > > > > > > > > > > > classes which are removed from  will 
> > > > > > > > > > > > > > follow, e.g:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > As these are a bunch of functions and types, in my 
> > > > > > > > > > > > > > eyes a check just for  is fine. But I'm 
> > > > > > > > > > > > > > also fine with a general 'deprecated API' check.
> > > > > > > > > > > > > > Alex, can you comment on this?
> > > > > > > > > > > > > There are already other checks for functions which 
> > > > > > > > > > > > > are removed in C++17 like 
> > > > > > > > > > > > > modernize-replace-random-shuffle.
> > > > > > > > > > > > > So I think having an separate check for functions and 
> > > > > > > > > > > > > types removed from  would be OK.
> > > > > > > > > > > > You've hit the nail on the head for what I'm trying to 
> > > > > > > > > > > > avoid -- we shouldn't have multiple checks unless they 
> > > > > > > > > > > > do drastically different things. Having a deprecated 
> > > > > > > > > > > > check like this really only makes sense for APIs that 
> > > > > > > > > > > > are deprecated but aren't uniformly marked as 
> > > > > > > > > > > > `[[deprecated]]` by the library. As such, I think we 
> > > > > > > > > > > > really only need one check for this rather than 
> > > > > > > > > > > > splitting it out over multiple checks -- the existing 
> > > > > > > > > > > > check functionality could be rolled into this one and 
> > > > > > > > > > > > its check become an alias.
> > > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > > > check.
> > > > > > > > > > > 
> > > > > > > > > > > IIUC, it should be possible to implement fixits at least 
> > > > > > > > > > > for some use cases here. My impression was that Jens was 
> > > > > > > > > > > at least considering to work on fixits. The other check 
> > > > > > > > > > > mentioned here - `modernize-replace-random-shuffle` 
> > > > > > > > > > > already implements fixits. Fixits are specific to the API 
> > > > > > > > > > > and some codebases may have better replacement APIs than 
> > > > > > > > > > > what the standard suggests, so different users may want 
> > > > > > > > > > > to apply different set of the fixes. Given all that, I 
> > > > > > > > > > > wouldn't just merge all of the checks dealing with 
> > > > > > > > > > > deprecated APIs. Splitting them at least by header seems 
> > > > > > > > > > > like a good start, maybe even finer granularity may be 
> > > > > > > > > > > needed in some cases.
> > > > > > > > > > TL;DR "they do drastically different things" is the 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG




Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > alexfh wrote:
> > > > > > > > > alexfh wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > > > > > ``? Just like we have a "deprecated 
> > > > > > > > > > > > > > headers" check, perhaps this should be a 
> > > > > > > > > > > > > > "deprecated APIs" check?
> > > > > > > > > > > > > Added full stop.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > > > >  or be extended to a full 'deprecated 
> > > > > > > > > > > > > API' check.
> > > > > > > > > > > > > This change is just a start, several more types and 
> > > > > > > > > > > > > classes which are removed from  will 
> > > > > > > > > > > > > follow, e.g:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > > > > > 
> > > > > > > > > > > > > As these are a bunch of functions and types, in my 
> > > > > > > > > > > > > eyes a check just for  is fine. But I'm 
> > > > > > > > > > > > > also fine with a general 'deprecated API' check.
> > > > > > > > > > > > > Alex, can you comment on this?
> > > > > > > > > > > > There are already other checks for functions which are 
> > > > > > > > > > > > removed in C++17 like modernize-replace-random-shuffle.
> > > > > > > > > > > > So I think having an separate check for functions and 
> > > > > > > > > > > > types removed from  would be OK.
> > > > > > > > > > > You've hit the nail on the head for what I'm trying to 
> > > > > > > > > > > avoid -- we shouldn't have multiple checks unless they do 
> > > > > > > > > > > drastically different things. Having a deprecated check 
> > > > > > > > > > > like this really only makes sense for APIs that are 
> > > > > > > > > > > deprecated but aren't uniformly marked as 
> > > > > > > > > > > `[[deprecated]]` by the library. As such, I think we 
> > > > > > > > > > > really only need one check for this rather than splitting 
> > > > > > > > > > > it out over multiple checks -- the existing check 
> > > > > > > > > > > functionality could be rolled into this one and its check 
> > > > > > > > > > > become an alias.
> > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > > check.
> > > > > > > > > > 
> > > > > > > > > > IIUC, it should be possible to implement fixits at least 
> > > > > > > > > > for some use cases here. My impression was that Jens was at 
> > > > > > > > > > least considering to work on fixits. The other check 
> > > > > > > > > > mentioned here - `modernize-replace-random-shuffle` already 
> > > > > > > > > > implements fixits. Fixits are specific to the API and some 
> > > > > > > > > > codebases may have better replacement APIs than what the 
> > > > > > > > > > standard suggests, so different users may want to apply 
> > > > > > > > > > different set of the fixes. Given all that, I wouldn't just 
> > > > > > > > > > merge all of the checks dealing with deprecated APIs. 
> > > > > > > > > > Splitting them at least by header seems like a good start, 
> > > > > > > > > > maybe even finer granularity may be needed in some cases.
> > > > > > > > > TL;DR "they do drastically different things" is the case for 
> > > > > > > > > this check and modernize-replace-random-shuffle.
> > > > > > > > I disagree that they do drastically different things or that 
> > > > > > > > fix-its are a problem. Some of these APIs have replacements, 
> > > > > > > > others do not. At the end of the day, the basics are the same: 
> > > > > > > > the functionality is deprecated and 

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

[Jens Massberg via Phabricator via cfe-commits]

massberg updated this revision to Diff 132592.
massberg marked an inline comment as done.

https://reviews.llvm.org/D42730

Files:
  clang-tidy/modernize/AvoidFunctionalCheck.cpp
  clang-tidy/modernize/AvoidFunctionalCheck.h
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-avoid-functional.rst
  test/clang-tidy/modernize-avoid-functional.cpp

Index: test/clang-tidy/modernize-avoid-functional.cpp
===
--- test/clang-tidy/modernize-avoid-functional.cpp
+++ test/clang-tidy/modernize-avoid-functional.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s modernize-avoid-functional %t
+
+namespace std {
+
+template 
+class binary_function {};
+
+template 
+class unary_function {};
+
+template 
+class pointer_to_unary_function
+: public std::unary_function {  // NOLINT
+};
+
+template 
+pointer_to_unary_function ptr_fun(Result (*f)(Arg)) {
+  pointer_to_unary_function Nothing;
+  return Nothing;
+}
+
+template 
+class mem_fun_t {};
+
+template 
+mem_fun_t mem_fun(Res (T::*f)()) {}
+
+}  // namespace std
+
+// CHECK-MESSAGES: [[@LINE+1]]:25: warning: 'binary_function' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class BinaryTestClass : public std::binary_function {
+ public:
+  BinaryTestClass();
+};
+
+// CHECK-MESSAGES: [[@LINE+1]]:24: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class UnaryTestClass : public std::unary_function {
+ public:
+  UnaryTestClass();
+};
+
+class TestClass {
+ public:
+  int test() { return 1; }
+};
+
+// CHECK-MESSAGES: [[@LINE+2]]:31: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class AnotherUnaryTestClass : public TestClass,
+  public std::unary_function {
+ public:
+  AnotherUnaryTestClass();
+};
+
+
+int simpleFunc(int X) { return X + 1; };
+
+void foo(void) {
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::ptr_fun' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+  std::ptr_fun(simpleFunc);
+
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::mem_fun' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+  std::mem_fun(::test);
+}
Index: docs/clang-tidy/checks/modernize-avoid-functional.rst
===
--- docs/clang-tidy/checks/modernize-avoid-functional.rst
+++ docs/clang-tidy/checks/modernize-avoid-functional.rst
@@ -0,0 +1,14 @@
+.. title:: clang-tidy - modernize-avoid-functional
+
+modernize-avoid-functional
+==
+
+Warns if types, classes and functions from '' header which are
+  deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is
+used:
+
+-  'std::unary_function'
+-  'std::binary_function'
+-  'std::ptr_fun'
+-  'std::mem_fun'
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -154,6 +154,7 @@
misc-unused-raii
misc-unused-using-decls
modernize-avoid-bind
+   modernize-avoid-functional
modernize-deprecated-headers
modernize-loop-convert
modernize-make-shared
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -89,6 +89,12 @@
   using decltype specifiers and lambda with otherwise unutterable 
   return types.
 
+- New `modernize-avoid-functional
+  `_ check
+
+  Warns if types, classes and functions from '' header which are
+  deprecated in C++11 and removed in C++17 are used.
+
 - New alias `hicpp-avoid-goto
   `_ to 
   `cppcoreguidelines-avoid-goto `_
Index: clang-tidy/modernize/ModernizeTidyModule.cpp
===
--- clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidBindCheck.h"
+#include "AvoidFunctionalCheck.h"
 #include "DeprecatedHeadersCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
@@ -45,6 +46,8 @@
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
 CheckFactories.registerCheck("modernize-avoid-bind");
+CheckFactories.registerCheck(
+"modernize-avoid-functional");
 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > Quuxplusone wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > alexfh wrote:
> > > > > > > > alexfh wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > massberg wrote:
> > > > > > > > > > > massberg wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > > > > ``? Just like we have a "deprecated 
> > > > > > > > > > > > > headers" check, perhaps this should be a "deprecated 
> > > > > > > > > > > > > APIs" check?
> > > > > > > > > > > > Added full stop.
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > > > check.
> > > > > > > > > > > > This change is just a start, several more types and 
> > > > > > > > > > > > classes which are removed from  will 
> > > > > > > > > > > > follow, e.g:
> > > > > > > > > > > > 
> > > > > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > > > > 
> > > > > > > > > > > > As these are a bunch of functions and types, in my eyes 
> > > > > > > > > > > > a check just for  is fine. But I'm also 
> > > > > > > > > > > > fine with a general 'deprecated API' check.
> > > > > > > > > > > > Alex, can you comment on this?
> > > > > > > > > > > There are already other checks for functions which are 
> > > > > > > > > > > removed in C++17 like modernize-replace-random-shuffle.
> > > > > > > > > > > So I think having an separate check for functions and 
> > > > > > > > > > > types removed from  would be OK.
> > > > > > > > > > You've hit the nail on the head for what I'm trying to 
> > > > > > > > > > avoid -- we shouldn't have multiple checks unless they do 
> > > > > > > > > > drastically different things. Having a deprecated check 
> > > > > > > > > > like this really only makes sense for APIs that are 
> > > > > > > > > > deprecated but aren't uniformly marked as `[[deprecated]]` 
> > > > > > > > > > by the library. As such, I think we really only need one 
> > > > > > > > > > check for this rather than splitting it out over multiple 
> > > > > > > > > > checks -- the existing check functionality could be rolled 
> > > > > > > > > > into this one and its check become an alias.
> > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > check.
> > > > > > > > > 
> > > > > > > > > IIUC, it should be possible to implement fixits at least for 
> > > > > > > > > some use cases here. My impression was that Jens was at least 
> > > > > > > > > considering to work on fixits. The other check mentioned here 
> > > > > > > > > - `modernize-replace-random-shuffle` already implements 
> > > > > > > > > fixits. Fixits are specific to the API and some codebases may 
> > > > > > > > > have better replacement APIs than what the standard suggests, 
> > > > > > > > > so different users may want to apply different set of the 
> > > > > > > > > fixes. Given all that, I wouldn't just merge all of the 
> > > > > > > > > checks dealing with deprecated APIs. Splitting them at least 
> > > > > > > > > by header seems like a good start, maybe even finer 
> > > > > > > > > granularity may be needed in some cases.
> > > > > > > > TL;DR "they do drastically different things" is the case for 
> > > > > > > > this check and modernize-replace-random-shuffle.
> > > > > > > I disagree that they do drastically different things or that 
> > > > > > > fix-its are a problem. Some of these APIs have replacements, 
> > > > > > > others do not. At the end of the day, the basics are the same: 
> > > > > > > the functionality is deprecated and you should consider a 
> > > > > > > replacement. Sometimes we know that replacement up front, other 
> > > > > > > times we don't. I don't think we should make users reach for a 
> > > > > > > per-header file answer to that problem unless it provides them 
> > > > > > 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:22
+  Finder->addMatcher(
+  cxxRecordDecl(allOf(anyOf(isDerivedFrom("std::binary_function"),
+isDerivedFrom("std::unary_function")),

aaron.ballman wrote:
> These should all be `::std::` instead of `std::` to cover pathological code 
> that puts `std` inside of another namespace.
Could you change the names to "::std" again, since my suggested code didn't 
take this comment into account?



Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > Quuxplusone wrote:
> > > > > aaron.ballman wrote:
> > > > > > alexfh wrote:
> > > > > > > alexfh wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > massberg wrote:
> > > > > > > > > > massberg wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > > > 
> > > > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > > > ``? Just like we have a "deprecated 
> > > > > > > > > > > > headers" check, perhaps this should be a "deprecated 
> > > > > > > > > > > > APIs" check?
> > > > > > > > > > > Added full stop.
> > > > > > > > > > > 
> > > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > > check.
> > > > > > > > > > > This change is just a start, several more types and 
> > > > > > > > > > > classes which are removed from  will follow, 
> > > > > > > > > > > e.g:
> > > > > > > > > > > 
> > > > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > > > 
> > > > > > > > > > > As these are a bunch of functions and types, in my eyes a 
> > > > > > > > > > > check just for  is fine. But I'm also fine 
> > > > > > > > > > > with a general 'deprecated API' check.
> > > > > > > > > > > Alex, can you comment on this?
> > > > > > > > > > There are already other checks for functions which are 
> > > > > > > > > > removed in C++17 like modernize-replace-random-shuffle.
> > > > > > > > > > So I think having an separate check for functions and types 
> > > > > > > > > > removed from  would be OK.
> > > > > > > > > You've hit the nail on the head for what I'm trying to avoid 
> > > > > > > > > -- we shouldn't have multiple checks unless they do 
> > > > > > > > > drastically different things. Having a deprecated check like 
> > > > > > > > > this really only makes sense for APIs that are deprecated but 
> > > > > > > > > aren't uniformly marked as `[[deprecated]]` by the library. 
> > > > > > > > > As such, I think we really only need one check for this 
> > > > > > > > > rather than splitting it out over multiple checks -- the 
> > > > > > > > > existing check functionality could be rolled into this one 
> > > > > > > > > and its check become an alias.
> > > > > > > > > I'm not sure if this check should be limited to  
> > > > > > > > > or be extended to a full 'deprecated API' check.
> > > > > > > > 
> > > > > > > > IIUC, it should be possible to implement fixits at least for 
> > > > > > > > some use cases here. My impression was that Jens was at least 
> > > > > > > > considering to work on fixits. The other check mentioned here - 
> > > > > > > > `modernize-replace-random-shuffle` already implements fixits. 
> > > > > > > > Fixits are specific to the API and some codebases may have 
> > > > > > > > better replacement APIs than what the standard suggests, so 
> > > > > > > > different users may want to apply different set of the fixes. 
> > > > > > > > Given all that, I wouldn't just merge all of the checks dealing 
> > > > > > > > with deprecated APIs. Splitting them at least by header seems 
> > > > > > > > like a good start, maybe even finer granularity may be needed 
> > > > > > > > in some cases.
> > > > > > > TL;DR "they do drastically different things" is the case for this 
> > > > > > > check and modernize-replace-random-shuffle.
> > > > > > I disagree that they do drastically different things or that 
> > > > > > fix-its are a problem. Some of these APIs have replacements, others 
> > > > > > do not. At the end of the day, the basics are 

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

[Jens Massberg via Phabricator via cfe-commits]

massberg marked 5 inline comments as done.
massberg added inline comments.



Comment at: test/clang-tidy/modernize-avoid-functional.cpp:30
+
+// CHECK-MESSAGES: [[@LINE+1]]:25: warning: 'binary_function' 
is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class BinaryTestClass : public std::binary_function {

alexfh wrote:
> Template parameters in this message are not useful. binary_function is 
> removed in C++17 regardless of the template parameters used. I played with 
> your patch a bit and  came up with a way to remove the template parameters in 
> a not overly hacky way (and also pulled out the common part of the message to 
> avoid duplication):
> 
> ```
> void AvoidFunctionalCheck::registerMatchers(MatchFinder *Finder) {
>   Finder->addMatcher(
>   cxxRecordDecl(allOf(isDerivedFrom(classTemplateSpecializationDecl(
> hasAnyName("std::binary_function",
>"std::unary_function"))
> .bind("base")),
>   anyOf(isClass(), ast_matchers::isStruct()),
>   ast_matchers::isDefinition()))
>   .bind("un_or_binary_derived"),
>   this);
>   Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::ptr_fun"
>  .bind("ptr_fun_call"),
>  this);
>   Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::mem_fun"
>  .bind("mem_fun_call"),
>  this);
> }
> 
> void AvoidFunctionalCheck::check(const MatchFinder::MatchResult ) {
>   const StringRef Message = "%0 is deprecated in C++11 and removed in C++17";
>   if (const auto *const Decl =
>   Result.Nodes.getNodeAs("un_or_binary_derived")) {
> const auto *SpecDecl =
> Result.Nodes.getNodeAs("base");
> for (CXXBaseSpecifier Base : Decl->bases()) {
>   if (SpecDecl == Base.getType()->getAsCXXRecordDecl())
> diag(Base.getLocStart(), Message) << 
> SpecDecl->getSpecializedTemplate();
> }
>   } else if (const auto *const Call =
>  Result.Nodes.getNodeAs("ptr_fun_call")) {
> diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
>   } else if (const auto *const Call =
>  Result.Nodes.getNodeAs("mem_fun_call")) {
> diag(Call->getLocStart(), Message) << "'std::mem_fun'";
>   }
> }
> ```
> 
> This could be done differently, of course, depending on which way this check 
> is going to evolutionize (e.g. if you're going to find usages of deprecated 
> entities on a lower level - say, TypeLocs, or if you're going to provide 
> fixits for narrower usage patterns).
Thanks for figuring this out! I updated to your suggested version.


https://reviews.llvm.org/D42730



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jens Massberg via Phabricator via cfe-commits]

massberg updated this revision to Diff 132538.
massberg added a comment.

Addressed comments of reviewers.


https://reviews.llvm.org/D42730

Files:
  clang-tidy/modernize/AvoidFunctionalCheck.cpp
  clang-tidy/modernize/AvoidFunctionalCheck.h
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-avoid-functional.rst
  test/clang-tidy/modernize-avoid-functional.cpp

Index: test/clang-tidy/modernize-avoid-functional.cpp
===
--- test/clang-tidy/modernize-avoid-functional.cpp
+++ test/clang-tidy/modernize-avoid-functional.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s modernize-avoid-functional %t
+
+namespace std {
+
+template 
+class binary_function {};
+
+template 
+class unary_function {};
+
+template 
+class pointer_to_unary_function
+: public std::unary_function {  // NOLINT
+};
+
+template 
+pointer_to_unary_function ptr_fun(Result (*f)(Arg)) {
+  pointer_to_unary_function Nothing;
+  return Nothing;
+}
+
+template 
+class mem_fun_t {};
+
+template 
+mem_fun_t mem_fun(Res (T::*f)()) {}
+
+}  // namespace std
+
+// CHECK-MESSAGES: [[@LINE+1]]:25: warning: 'binary_function' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class BinaryTestClass : public std::binary_function {
+ public:
+  BinaryTestClass();
+};
+
+// CHECK-MESSAGES: [[@LINE+1]]:24: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class UnaryTestClass : public std::unary_function {
+ public:
+  UnaryTestClass();
+};
+
+class TestClass {
+ public:
+  int test() { return 1; }
+};
+
+// CHECK-MESSAGES: [[@LINE+2]]:31: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+class AnotherUnaryTestClass : public TestClass,
+  public std::unary_function {
+ public:
+  AnotherUnaryTestClass();
+};
+
+
+int simpleFunc(int X) { return X + 1; };
+
+void foo(void) {
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::ptr_fun' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+  std::ptr_fun(simpleFunc);
+
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::mem_fun' is deprecated in C++11 and removed in C++17 [modernize-avoid-functional]
+  std::mem_fun(::test);
+}
Index: docs/clang-tidy/checks/modernize-avoid-functional.rst
===
--- docs/clang-tidy/checks/modernize-avoid-functional.rst
+++ docs/clang-tidy/checks/modernize-avoid-functional.rst
@@ -0,0 +1,14 @@
+.. title:: clang-tidy - modernize-avoid-functional
+
+modernize-avoid-functional
+==
+
+Warns if types, classes and functions from '' header which are
+  deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is
+used:
+
+-  'std::unary_function'
+-  'std::binary_function'
+-  'std::ptr_fun'
+-  'std::mem_fun'
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -154,6 +154,7 @@
misc-unused-raii
misc-unused-using-decls
modernize-avoid-bind
+   modernize-avoid-functional
modernize-deprecated-headers
modernize-loop-convert
modernize-make-shared
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -89,6 +89,12 @@
   using decltype specifiers and lambda with otherwise unutterable 
   return types.
 
+- New `modernize-avoid-functional
+  `_ check
+
+  Warns if types, classes and functions from '' header which are
+  deprecated in C++11 and removed in C++17 are used.
+
 - New alias `hicpp-avoid-goto
   `_ to 
   `cppcoreguidelines-avoid-goto `_
Index: clang-tidy/modernize/ModernizeTidyModule.cpp
===
--- clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidBindCheck.h"
+#include "AvoidFunctionalCheck.h"
 #include "DeprecatedHeadersCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
@@ -45,6 +46,8 @@
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
 CheckFactories.registerCheck("modernize-avoid-bind");
+CheckFactories.registerCheck(
+"modernize-avoid-functional");

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > Quuxplusone wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > alexfh wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > massberg wrote:
> > > > > > > > > massberg wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > > 
> > > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > > ``? Just like we have a "deprecated headers" 
> > > > > > > > > > > check, perhaps this should be a "deprecated APIs" check?
> > > > > > > > > > Added full stop.
> > > > > > > > > > 
> > > > > > > > > > I'm not sure if this check should be limited to 
> > > > > > > > > >  or be extended to a full 'deprecated API' 
> > > > > > > > > > check.
> > > > > > > > > > This change is just a start, several more types and classes 
> > > > > > > > > > which are removed from  will follow, e.g:
> > > > > > > > > > 
> > > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > > 
> > > > > > > > > > As these are a bunch of functions and types, in my eyes a 
> > > > > > > > > > check just for  is fine. But I'm also fine with 
> > > > > > > > > > a general 'deprecated API' check.
> > > > > > > > > > Alex, can you comment on this?
> > > > > > > > > There are already other checks for functions which are 
> > > > > > > > > removed in C++17 like modernize-replace-random-shuffle.
> > > > > > > > > So I think having an separate check for functions and types 
> > > > > > > > > removed from  would be OK.
> > > > > > > > You've hit the nail on the head for what I'm trying to avoid -- 
> > > > > > > > we shouldn't have multiple checks unless they do drastically 
> > > > > > > > different things. Having a deprecated check like this really 
> > > > > > > > only makes sense for APIs that are deprecated but aren't 
> > > > > > > > uniformly marked as `[[deprecated]]` by the library. As such, I 
> > > > > > > > think we really only need one check for this rather than 
> > > > > > > > splitting it out over multiple checks -- the existing check 
> > > > > > > > functionality could be rolled into this one and its check 
> > > > > > > > become an alias.
> > > > > > > > I'm not sure if this check should be limited to  or 
> > > > > > > > be extended to a full 'deprecated API' check.
> > > > > > > 
> > > > > > > IIUC, it should be possible to implement fixits at least for some 
> > > > > > > use cases here. My impression was that Jens was at least 
> > > > > > > considering to work on fixits. The other check mentioned here - 
> > > > > > > `modernize-replace-random-shuffle` already implements fixits. 
> > > > > > > Fixits are specific to the API and some codebases may have better 
> > > > > > > replacement APIs than what the standard suggests, so different 
> > > > > > > users may want to apply different set of the fixes. Given all 
> > > > > > > that, I wouldn't just merge all of the checks dealing with 
> > > > > > > deprecated APIs. Splitting them at least by header seems like a 
> > > > > > > good start, maybe even finer granularity may be needed in some 
> > > > > > > cases.
> > > > > > TL;DR "they do drastically different things" is the case for this 
> > > > > > check and modernize-replace-random-shuffle.
> > > > > I disagree that they do drastically different things or that fix-its 
> > > > > are a problem. Some of these APIs have replacements, others do not. 
> > > > > At the end of the day, the basics are the same: the functionality is 
> > > > > deprecated and you should consider a replacement. Sometimes we know 
> > > > > that replacement up front, other times we don't. I don't think we 
> > > > > should make users reach for a per-header file answer to that problem 
> > > > > unless it provides them some benefit. I don't see users caring to 
> > > > > update  but not other headers.
> > > > > 
> > > > > I can see benefit to splitting the *implementations* of the checks 
> > > > > along arbitrary lines, but how we structure the implementation is 
> > > > > orthogonal to how we surface the functionality.
> > > > This sounds like clang-tidy ought to have an umbrella option here, 
> > > > analogous to how -Wformat 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

aaron.ballman wrote:
> alexfh wrote:
> > Quuxplusone wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > alexfh wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > massberg wrote:
> > > > > > > > massberg wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > > 
> > > > > > > > > > Why should this modernize check be limited to 
> > > > > > > > > > ``? Just like we have a "deprecated headers" 
> > > > > > > > > > check, perhaps this should be a "deprecated APIs" check?
> > > > > > > > > Added full stop.
> > > > > > > > > 
> > > > > > > > > I'm not sure if this check should be limited to  
> > > > > > > > > or be extended to a full 'deprecated API' check.
> > > > > > > > > This change is just a start, several more types and classes 
> > > > > > > > > which are removed from  will follow, e.g:
> > > > > > > > > 
> > > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > > 
> > > > > > > > > As these are a bunch of functions and types, in my eyes a 
> > > > > > > > > check just for  is fine. But I'm also fine with a 
> > > > > > > > > general 'deprecated API' check.
> > > > > > > > > Alex, can you comment on this?
> > > > > > > > There are already other checks for functions which are removed 
> > > > > > > > in C++17 like modernize-replace-random-shuffle.
> > > > > > > > So I think having an separate check for functions and types 
> > > > > > > > removed from  would be OK.
> > > > > > > You've hit the nail on the head for what I'm trying to avoid -- 
> > > > > > > we shouldn't have multiple checks unless they do drastically 
> > > > > > > different things. Having a deprecated check like this really only 
> > > > > > > makes sense for APIs that are deprecated but aren't uniformly 
> > > > > > > marked as `[[deprecated]]` by the library. As such, I think we 
> > > > > > > really only need one check for this rather than splitting it out 
> > > > > > > over multiple checks -- the existing check functionality could be 
> > > > > > > rolled into this one and its check become an alias.
> > > > > > > I'm not sure if this check should be limited to  or 
> > > > > > > be extended to a full 'deprecated API' check.
> > > > > > 
> > > > > > IIUC, it should be possible to implement fixits at least for some 
> > > > > > use cases here. My impression was that Jens was at least 
> > > > > > considering to work on fixits. The other check mentioned here - 
> > > > > > `modernize-replace-random-shuffle` already implements fixits. 
> > > > > > Fixits are specific to the API and some codebases may have better 
> > > > > > replacement APIs than what the standard suggests, so different 
> > > > > > users may want to apply different set of the fixes. Given all that, 
> > > > > > I wouldn't just merge all of the checks dealing with deprecated 
> > > > > > APIs. Splitting them at least by header seems like a good start, 
> > > > > > maybe even finer granularity may be needed in some cases.
> > > > > TL;DR "they do drastically different things" is the case for this 
> > > > > check and modernize-replace-random-shuffle.
> > > > I disagree that they do drastically different things or that fix-its 
> > > > are a problem. Some of these APIs have replacements, others do not. At 
> > > > the end of the day, the basics are the same: the functionality is 
> > > > deprecated and you should consider a replacement. Sometimes we know 
> > > > that replacement up front, other times we don't. I don't think we 
> > > > should make users reach for a per-header file answer to that problem 
> > > > unless it provides them some benefit. I don't see users caring to 
> > > > update  but not other headers.
> > > > 
> > > > I can see benefit to splitting the *implementations* of the checks 
> > > > along arbitrary lines, but how we structure the implementation is 
> > > > orthogonal to how we surface the functionality.
> > > This sounds like clang-tidy ought to have an umbrella option here, 
> > > analogous to how -Wformat turns on -Wformat-security, 
> > > -Wformat-truncation, -Wformat-overflow, etc. (Well, in GCC it doesn't, 
> > > but that's the general idea.)
> > > So there could be a 'modernize-avoid-deprecated-in-c++11' umbrella option 
> > > that 

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from 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/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

alexfh wrote:
> Quuxplusone wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > alexfh wrote:
> > > > > aaron.ballman wrote:
> > > > > > massberg wrote:
> > > > > > > massberg wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > > 
> > > > > > > > > Why should this modernize check be limited to ``? 
> > > > > > > > > Just like we have a "deprecated headers" check, perhaps this 
> > > > > > > > > should be a "deprecated APIs" check?
> > > > > > > > Added full stop.
> > > > > > > > 
> > > > > > > > I'm not sure if this check should be limited to  or 
> > > > > > > > be extended to a full 'deprecated API' check.
> > > > > > > > This change is just a start, several more types and classes 
> > > > > > > > which are removed from  will follow, e.g:
> > > > > > > > 
> > > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > > - std::bind1st, std::bind2nd
> > > > > > > > - std::unary_function, std::binary_function
> > > > > > > > - std::pointer_to_unary_function, 
> > > > > > > > std::pointer_to_binary_function, std::mem_fun_t, 
> > > > > > > > std::mem_fun1_t, std::const_mem_fun_t, 
> > > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, 
> > > > > > > > std::mem_fun1_ref_t, std::const_mem_fun_ref_t, 
> > > > > > > > std::const_mem_fun1_ref_t
> > > > > > > > - std::binder1st, std::binder2nd
> > > > > > > > 
> > > > > > > > As these are a bunch of functions and types, in my eyes a check 
> > > > > > > > just for  is fine. But I'm also fine with a general 
> > > > > > > > 'deprecated API' check.
> > > > > > > > Alex, can you comment on this?
> > > > > > > There are already other checks for functions which are removed in 
> > > > > > > C++17 like modernize-replace-random-shuffle.
> > > > > > > So I think having an separate check for functions and types 
> > > > > > > removed from  would be OK.
> > > > > > You've hit the nail on the head for what I'm trying to avoid -- we 
> > > > > > shouldn't have multiple checks unless they do drastically different 
> > > > > > things. Having a deprecated check like this really only makes sense 
> > > > > > for APIs that are deprecated but aren't uniformly marked as 
> > > > > > `[[deprecated]]` by the library. As such, I think we really only 
> > > > > > need one check for this rather than splitting it out over multiple 
> > > > > > checks -- the existing check functionality could be rolled into 
> > > > > > this one and its check become an alias.
> > > > > > I'm not sure if this check should be limited to  or be 
> > > > > > extended to a full 'deprecated API' check.
> > > > > 
> > > > > IIUC, it should be possible to implement fixits at least for some use 
> > > > > cases here. My impression was that Jens was at least considering to 
> > > > > work on fixits. The other check mentioned here - 
> > > > > `modernize-replace-random-shuffle` already implements fixits. Fixits 
> > > > > are specific to the API and some codebases may have better 
> > > > > replacement APIs than what the standard suggests, so different users 
> > > > > may want to apply different set of the fixes. Given all that, I 
> > > > > wouldn't just merge all of the checks dealing with deprecated APIs. 
> > > > > Splitting them at least by header seems like a good start, maybe even 
> > > > > finer granularity may be needed in some cases.
> > > > TL;DR "they do drastically different things" is the case for this check 
> > > > and modernize-replace-random-shuffle.
> > > I disagree that they do drastically different things or that fix-its are 
> > > a problem. Some of these APIs have replacements, others do not. At the 
> > > end of the day, the basics are the same: the functionality is deprecated 
> > > and you should consider a replacement. Sometimes we know that replacement 
> > > up front, other times we don't. I don't think we should make users reach 
> > > for a per-header file answer to that problem unless it provides them some 
> > > benefit. I don't see users caring to update  but not other 
> > > headers.
> > > 
> > > I can see benefit to splitting the *implementations* of the checks along 
> > > arbitrary lines, but how we structure the implementation is orthogonal to 
> > > how we surface the functionality.
> > This sounds like clang-tidy ought to have an umbrella option here, 
> > analogous to how -Wformat turns on -Wformat-security, -Wformat-truncation, 
> > -Wformat-overflow, etc. (Well, in GCC it doesn't, but that's the general 
> > idea.)
> > So there could be a 'modernize-avoid-deprecated-in-c++11' umbrella option 
> > that turns on both 'modernize-replace-random-shuffle' and 
> > 'modernize-avoid-functional'; a 'modernize-avoid-removed-in-c++17' umbrella 
> > option that turns on those two plus 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from  header
+///

Quuxplusone wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > massberg wrote:
> > > > > > massberg wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Missing full stop at the end of the sentence.
> > > > > > > > 
> > > > > > > > Why should this modernize check be limited to ``? 
> > > > > > > > Just like we have a "deprecated headers" check, perhaps this 
> > > > > > > > should be a "deprecated APIs" check?
> > > > > > > Added full stop.
> > > > > > > 
> > > > > > > I'm not sure if this check should be limited to  or 
> > > > > > > be extended to a full 'deprecated API' check.
> > > > > > > This change is just a start, several more types and classes which 
> > > > > > > are removed from  will follow, e.g:
> > > > > > > 
> > > > > > > - std::ptr_fun, std::mem_fun, std::mem_fun_ref
> > > > > > > - std::bind1st, std::bind2nd
> > > > > > > - std::unary_function, std::binary_function
> > > > > > > - std::pointer_to_unary_function, 
> > > > > > > std::pointer_to_binary_function, std::mem_fun_t, std::mem_fun1_t, 
> > > > > > > std::const_mem_fun_t, 
> > > > > > > - std::const_mem_fun1_t, std::mem_fun_ref_t, std::mem_fun1_ref_t, 
> > > > > > > std::const_mem_fun_ref_t, std::const_mem_fun1_ref_t
> > > > > > > - std::binder1st, std::binder2nd
> > > > > > > 
> > > > > > > As these are a bunch of functions and types, in my eyes a check 
> > > > > > > just for  is fine. But I'm also fine with a general 
> > > > > > > 'deprecated API' check.
> > > > > > > Alex, can you comment on this?
> > > > > > There are already other checks for functions which are removed in 
> > > > > > C++17 like modernize-replace-random-shuffle.
> > > > > > So I think having an separate check for functions and types removed 
> > > > > > from  would be OK.
> > > > > You've hit the nail on the head for what I'm trying to avoid -- we 
> > > > > shouldn't have multiple checks unless they do drastically different 
> > > > > things. Having a deprecated check like this really only makes sense 
> > > > > for APIs that are deprecated but aren't uniformly marked as 
> > > > > `[[deprecated]]` by the library. As such, I think we really only need 
> > > > > one check for this rather than splitting it out over multiple checks 
> > > > > -- the existing check functionality could be rolled into this one and 
> > > > > its check become an alias.
> > > > > I'm not sure if this check should be limited to  or be 
> > > > > extended to a full 'deprecated API' check.
> > > > 
> > > > IIUC, it should be possible to implement fixits at least for some use 
> > > > cases here. My impression was that Jens was at least considering to 
> > > > work on fixits. The other check mentioned here - 
> > > > `modernize-replace-random-shuffle` already implements fixits. Fixits 
> > > > are specific to the API and some codebases may have better replacement 
> > > > APIs than what the standard suggests, so different users may want to 
> > > > apply different set of the fixes. Given all that, I wouldn't just merge 
> > > > all of the checks dealing with deprecated APIs. Splitting them at least 
> > > > by header seems like a good start, maybe even finer granularity may be 
> > > > needed in some cases.
> > > TL;DR "they do drastically different things" is the case for this check 
> > > and modernize-replace-random-shuffle.
> > I disagree that they do drastically different things or that fix-its are a 
> > problem. Some of these APIs have replacements, others do not. At the end of 
> > the day, the basics are the same: the functionality is deprecated and you 
> > should consider a replacement. Sometimes we know that replacement up front, 
> > other times we don't. I don't think we should make users reach for a 
> > per-header file answer to that problem unless it provides them some 
> > benefit. I don't see users caring to update  but not other 
> > headers.
> > 
> > I can see benefit to splitting the *implementations* of the checks along 
> > arbitrary lines, but how we structure the implementation is orthogonal to 
> > how we surface the functionality.
> This sounds like clang-tidy ought to have an umbrella option here, analogous 
> to how -Wformat turns on -Wformat-security, -Wformat-truncation, 
> -Wformat-overflow, etc. (Well, in GCC it doesn't, but that's the general 
> idea.)
> So there could be a 'modernize-avoid-deprecated-in-c++11' umbrella option 
> that turns on both 'modernize-replace-random-shuffle' and 
> 'modernize-avoid-functional'; a 'modernize-avoid-removed-in-c++17' umbrella 
> option that turns on those two plus some other options; and so on.
> Just a thought. If such a structure is anathema to how clang-tidy does 
> things, then never mind. :)
> I don't see users caring to update  but not other headers.