[clang-tidy] Prefer transparent functors to non-transparent one.

[xazax.hun]

Summary

This check finds the usages of non-transparent functors and suggest to use the transparent ones.

[aaron.ballman]

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

27

Should we make this a configurable list that users can add to?

62

This diagnostic is too terse; anyone that is unaware of what a transparent functor is will likely be stumped by it, especially since there is no fixit.

Since this is the case where we cannot be sure that a transparent functor is the correct solution, should this be enabled via an option (default on)?

94

This diagnostic could stand to be less terse as well, IMO. The fixit helps though.

docs/clang-tidy/checks/modernize-use-transparent-functors.rst

7

Using transparent functors the -> When using transparent functors, the

[xazax.hun]

• Fixed the performance problems.
• Altered the diagnostic text.
• Documentation improvements.
• Added an option to silence some warnings.
• Updated to latest trunk.

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

27

I am not sure how frequent is that somebody would like to add some types to this list, but it can be added in a follow up patch.

62

I also extended the error message to refer to the alternative name (diamond operators) as well.

I did add an option but I am not happy with the name of the option. Do you have a suggestion?

[xazax.hun]

I attached the results of this check for LLVM.

results.txt553 KBDownload

[Prazek]

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

71

The message would be much better if you would put the name of this functor, like
"prefer transparent functor (%0)" where %0 would be evaluated to 'std::greater<>" etc.

90–110

This can be moved to one or 2 functions, returning FunctorTypeLoc or llvm::Optional<TemplateSpecializationTypeLoc>

docs/clang-tidy/checks/modernize-use-transparent-functors.rst

33–34

I think
... will not warn on these cases as shown above, where automatic FIXIT...
would have been better.

34

Add a note
This check requires C++14 or higher to run.

[Prazek]

docs/clang-tidy/checks/modernize-use-transparent-functors.rst

13

Say somewhere that you also handle cases like
std::less<Type>(arg1, arg2)
because from this documentation I didn't know about it.

[aaron.ballman]

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

27

I'm fine with a follow-on patch.

62

SafeMode is a bit dramatic-sounding, but I can't come up with something better, so it's probably fine.

[xazax.hun]

• Addressed review comments.

[aaron.ballman]

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

71

You should quote the %0 to clarify that you're referring to syntax.

89

Is the .str() required? (Same question applies below.)

[xazax.hun]

• Updated to latest TOT
• Fixed review comments.

[xazax.hun]

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

89

It does not compile without it. It looks like the << is not overloaded for Twine.

[aaron.ballman]

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

89

I learn something new every day. ;-)

docs/clang-tidy/checks/modernize-use-transparent-functors.rst

9

It not possible -> It is not possible

36–38

The way I read this, it seems to be claiming that if you set SafeMode to nonzero, the check never warns. How about:

If the option is set to non-zero, the check will not diagnose cases where using a transparent functor cannot be guaranteed to produce identical results as the original code. The default value for this option is `0`.

[xazax.hun]

Thanks for the reviews, committed in https://reviews.llvm.org/rL287107 .

clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   ReplaceAutoPtrCheck.cpp
   ShrinkToFitCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultCheck.cpp
   UseEmplaceCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
+  UseTransparentFunctorsCheck.cpp
   UseUsingCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   )

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
+#include "UseTransparentFunctorsCheck.h"
 #include "UseUsingCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultCheck>("modernize-use-default");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
+    CheckFactories.registerCheck<UseTransparentFunctorsCheck>(
+        "modernize-use-transparent-functors");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;
     // For types whose size in bytes is above this threshold, we prefer taking a
     // const-reference than making a copy.

clang-tidy/modernize/UseTransparentFunctorsCheck.h

+//===--- UseTransparentFunctorsCheck.h - clang-tidy--------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_TRANSPARENT_FUNCTORS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_TRANSPARENT_FUNCTORS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Prefer using transparent functors to non-transparent ones.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-transparent-functors.html
+class UseTransparentFunctorsCheck : public ClangTidyCheck {
+public:
+  UseTransparentFunctorsCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+private:
+  const bool SafeMode;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_TRANSPARENT_FUNCTORS_H

clang-tidy/modernize/UseTransparentFunctorsCheck.cpp

+//===--- UseTransparentFunctorsCheck.cpp - clang-tidy----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseTransparentFunctorsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+UseTransparentFunctorsCheck::UseTransparentFunctorsCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), SafeMode(Options.get("SafeMode", 0)) {}
+
+void UseTransparentFunctorsCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "SafeMode", SafeMode ? 1 : 0);
+}
+
+void UseTransparentFunctorsCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus14)
+    return;
+
+  const auto TransparentFunctors =
+      classTemplateSpecializationDecl(
+          unless(hasAnyTemplateArgument(refersToType(voidType()))),
+          hasAnyName("::std::plus", "::std::minus", "::std::multiplies",
+                     "::std::divides", "::std::modulus", "::std::negate",
+                     "::std::equal_to", "::std::not_equal_to", "::std::greater",
+                     "::std::less", "::std::greater_equal", "::std::less_equal",
+                     "::std::logical_and", "::std::logical_or",
+                     "::std::logical_not", "::std::bit_and", "::std::bit_or",
+                     "::std::bit_xor", "::std::bit_not"))
+          .bind("FunctorClass");
+
+  // Non-transparent functor mentioned as a template parameter. FIXIT.
+  Finder->addMatcher(
+      loc(qualType(
+              unless(elaboratedType()),
+              hasDeclaration(classTemplateSpecializationDecl(
+                  unless(hasAnyTemplateArgument(templateArgument(refersToType(
+                      qualType(pointsTo(qualType(isAnyCharacter()))))))),
+                  hasAnyTemplateArgument(
+                      templateArgument(refersToType(qualType(hasDeclaration(
+                                           TransparentFunctors))))
+                          .bind("Functor"))))))
+          .bind("FunctorParentLoc"),
+      this);
+
+  if (SafeMode)
+    return;
+
+  // Non-transparent functor constructed. No FIXIT. There is no easy way
+  // to rule out the problematic char* vs string case.
+  Finder->addMatcher(cxxConstructExpr(hasDeclaration(cxxMethodDecl(
+                                          ofClass(TransparentFunctors))),
+                                      unless(isInTemplateInstantiation()))
+                         .bind("FuncInst"),
+                     this);
+}
+
+static const StringRef Message = "prefer transparent functors (%0)";
+
+template <typename T> static T getInnerTypeLocAs(TypeLoc Loc) {
+  T Result;
+  while (Result.isNull() && !Loc.isNull()) {
+    Result = Loc.getAs<T>();
+    Loc = Loc.getNextTypeLoc();
+  }
+  return Result;
+}
+
+void UseTransparentFunctorsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *FuncClass =
+      Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("FunctorClass");
+  if (const auto *FuncInst =
+          Result.Nodes.getNodeAs<CXXConstructExpr>("FuncInst")) {
+    diag(FuncInst->getLocStart(), Message)
+        << (FuncClass->getName() + "<>").str();
+    return;
+  }
+
+  const auto *Functor = Result.Nodes.getNodeAs<TemplateArgument>("Functor");
+  const auto FunctorParentLoc =
+      Result.Nodes.getNodeAs<TypeLoc>("FunctorParentLoc")
+          ->getAs<TemplateSpecializationTypeLoc>();
+
+  if (!FunctorParentLoc)
+    return;
+
+  unsigned ArgNum = 0;
+  const auto *FunctorParentType =
+      FunctorParentLoc.getType()->castAs<TemplateSpecializationType>();
+  for (; ArgNum < FunctorParentType->getNumArgs(); ++ArgNum) {
+    const TemplateArgument &Arg = FunctorParentType->getArg(ArgNum);
+    if (Arg.getKind() != TemplateArgument::Type)
+      continue;
+    QualType ParentArgType = Arg.getAsType();
+    if (ParentArgType->isRecordType() &&
+        ParentArgType->getAsCXXRecordDecl() ==
+            Functor->getAsType()->getAsCXXRecordDecl())
+      break;
+  }
+  // Functor is a default template argument.
+  if (ArgNum == FunctorParentType->getNumArgs())
+    return;
+  TemplateArgumentLoc FunctorLoc = FunctorParentLoc.getArgLoc(ArgNum);
+  auto FunctorTypeLoc = getInnerTypeLocAs<TemplateSpecializationTypeLoc>(
+      FunctorLoc.getTypeSourceInfo()->getTypeLoc());
+  if (FunctorTypeLoc.isNull())
+    return;
+
+  SourceLocation ReportLoc = FunctorLoc.getLocation();
+  diag(ReportLoc, Message) << (FuncClass->getName() + "<>").str()
+                           << FixItHint::CreateRemoval(
+                                  FunctorTypeLoc.getArgLoc(0).getSourceRange());
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- `modernize-make-unique
   and `modernize-make-shared
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_
   now handle calls to the smart pointer's ``reset()`` method.
 
 - The `modernize-use-auto
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>`_ check
   now warns about variable declarations that are initialized with a cast.
 
+- New `modernize-use-transparent-functors
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-transparent-functors.html>`_ check
+
+  Replaces uses of non-transparent functors with transparent ones where applicable.
+
 - New `mpi-buffer-deref
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-buffer-deref.html>`_ check
 
   Flags buffers which are insufficiently dereferenced when passed to an MPI function call.
 
 - New `mpi-type-mismatch
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-type-mismatch.html>`_ check
 

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default
    modernize-use-emplace
    modernize-use-nullptr
    modernize-use-override
+   modernize-use-transparent-functors
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
    performance-inefficient-string-concatenation
    performance-unnecessary-copy-initialization

docs/clang-tidy/checks/modernize-use-transparent-functors.rst

+.. title:: clang-tidy - modernize-use-transparent-functors
+
+modernize-use-transparent-functors
+==================================
+
+Prefer transparent functors to non-transparent ones. When using transparent
+functors, the type does not need to be repeated. The code is easier to read,
+maintain and less prone to errors. It not possible to introduce unwanted
+conversions.
+
+  .. code-block:: c++
+
+    // Non-transparent functor  
+    std::map<int, std::string, std::greater<int>> s;
+
+    // Transparent functor.
+    std::map<int, std::string, std::greater<>> s;
+
+    // Non-transparent functor
+    using MyFunctor = std::less<MyType>;
+
+It is not always a safe transformation though. The following case will be 
+untouched to preserve the semantics.
+
+  .. code-block:: c++
+
+    // Non-transparent functor  
+    std::map<const char *, std::string, std::greater<std::string>> s;
+
+Options
+-------
+
+.. option:: SafeMode
+
+   If the option is set to non-zero (default is `0`), the check will not
+   warn on these cases as shown above, where automatic FIXIT is not safe to
+   apply.
+
+This check requires using C++14 or higher to run.

test/clang-tidy/modernize-use-transparent-functors.cpp

+// RUN: %check_clang_tidy %s modernize-use-transparent-functors %t -- -- -std=c++14
+
+namespace std {
+template<class T>
+struct remove_reference;
+
+template <class T>
+constexpr T &&forward(typename std::remove_reference<T>::type &t);
+
+template <class T>
+constexpr T &&forward(typename std::remove_reference<T>::type &&t);
+
+template <typename T = void>
+struct plus {
+  constexpr T operator()(const T &Lhs, const T &Rhs) const;
+};
+
+template <>
+struct plus<void> {
+  template <typename T, typename U>
+  constexpr auto operator()(T &&Lhs, U &&Rhs) const -> 
+    decltype(forward<T>(Lhs) + forward<U>(Rhs));
+};
+
+template <typename T = void>
+struct less {
+  constexpr bool operator()(const T &Lhs, const T &Rhs) const;
+};
+
+template <>
+struct less<void> {
+  template <typename T, typename U>
+  constexpr bool operator()(T &&Lhs, U &&Rhs) const;
+};
+
+template <typename T = void>
+struct logical_not {
+  constexpr bool operator()(const T &Arg) const;
+};
+
+template <>
+struct logical_not<void> {
+  template <typename T>
+  constexpr bool operator()(T &&Arg) const;
+};
+
+template <typename T>
+class allocator;
+
+template <
+    class Key,
+    class Compare = std::less<>,
+    class Allocator = std::allocator<Key>>
+class set {};
+
+template <
+    class Key,
+    class Compare = std::less<Key>,
+    class Allocator = std::allocator<Key>>
+class set2 {};
+
+template <class InputIt, class UnaryPredicate>
+InputIt find_if(InputIt first, InputIt last,
+                UnaryPredicate p);
+
+template <class RandomIt, class Compare>
+void sort(RandomIt first, RandomIt last, Compare comp);
+
+class iterator {};
+class string {};
+}
+
+int main() {
+  using std::set;
+  using std::less;
+  std::set<int, std::less<int>> s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer transparent functors (less<>) [modernize-use-transparent-functors]
+  // CHECK-FIXES: {{^}}  std::set<int, std::less<>> s;{{$}}
+  set<int, std::less<int>> s2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: prefer transparent functors
+  // CHECK-FIXES: {{^}}  set<int, std::less<>> s2;{{$}}
+  set<int, less<int>> s3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: prefer transparent functors
+  // CHECK-FIXES: {{^}}  set<int, less<>> s3;{{$}}
+  std::set<int, std::less<>> s4;
+  std::set<char *, std::less<std::string>> s5;
+  std::set<set<int, less<int>>, std::less<>> s6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: prefer transparent functors
+  // CHECK-FIXES: {{^}}  std::set<set<int, less<>>, std::less<>> s6;{{$}}
+  std::iterator begin, end;
+  sort(begin, end, std::less<int>());
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: prefer transparent functors
+  std::sort(begin, end, std::less<>());
+  find_if(begin, end, std::logical_not<bool>());
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer transparent functors
+  std::find_if(begin, end, std::logical_not<>());
+  using my_set = std::set<int, std::less<int>>;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: prefer transparent functors
+  // CHECK-FIXES: {{^}}  using my_set = std::set<int, std::less<>>;{{$}}
+  using my_set2 = std::set<char*, std::less<std::string>>;
+  using my_less = std::less<std::string>;
+  find_if(begin, end, my_less());
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer transparent functors
+  std::set2<int> control;
+}
+
+