[clang-tidy] performance-* checks: Match AllowedTypes against qualified type names when they contain "::".

[flx]

Summary

This allows users to be more precise and exclude a type in a specific namespace
from triggering the check instead of excluding all types with the same
unqualified name.

This change should not interfere with correctly configured clang-tidy setups
since an AllowedType with "::" would never match.

[ymandel]

(but please wait for "accept" from an owner).

clang-tools-extra/clang-tidy/utils/Matchers.h

74

nit: while this change is not much worse than the existing code, the matcher really should perform this test for "::", and create the Regex, for each string just once. Regex construction is relatively expensive, and matchers of this sort (very generic) are often called quite frequently.

for that matter, this matcher is now really close to matchesName and would probably be best as a first-class matcher, rather than limited to clang-tidy/utils. But, I'll admit that such is beyond the scope of this patch.

[flx]

clang-tools-extra/clang-tidy/utils/Matchers.h

74

It wouldn't be hard to change it something like this:

struct MatcherPair {                                                                                                       
    llvm::Regex regex;                                                                                                       
    bool matchQualifiedName = false;                                                                                         
  };                                                                                                                         
  std::vector<MatcherPair> Matchers;                                                                                         
  std::transform(                                                                                                            
      NameList.begin(), NameList.end(), std::back_inserter(Matchers),                                                        
      [](const std::string& Name) {                                                                                          
        return MatcherPair{                                                                                                  
            .regex = llvm::Regex(Name),                                                                                      
            .matchQualifiedName = Name.find("::") != std::string::npos,                                                      
        };                                                                                                                   
      });                                                                                                                    
  return llvm::any_of(Matchers, [&Node](const MatcherPair& Matcher) {                                                        
    if (Matcher.matchQualifiedName) {                                                                                        
      return Matcher.regex.match(Node.getQualifiedNameAsString());                                                           
    }                                                                                                                        
    return Matcher.regex.match(Node.getName());                                                                              
  });

Is this what you had in mind?

[ymandel]

clang-tools-extra/clang-tidy/utils/Matchers.h

74

Thanks, that's almost it. The key point is that the code before the return be executed once, during matcher construction, and not each time match is called. You could define your own class (instead of using the macro) or just your own factory function:

struct MatcherPair {                                                                                                       
    llvm::Regex regex;                                                                                                       
    bool matchQualifiedName = false;                                                                                         
};        

AST_MATCHER_P(NamedDecl, matchesAnyListedNameImpl, std::vector<MatcherPair>, Matchers) {
  return llvm::any_of(Matchers, [&Node](const MatcherPair& Matcher) {                                                        
    if (Matcher.matchQualifiedName) {                                                                                        
      return Matcher.regex.match(Node.getQualifiedNameAsString());                                                           
    }                                                                                                                        
    return Matcher.regex.match(Node.getName());                                                                              
  });
}

Matcher<NamedDecl> matchesAnyListedName(std::vector<std::string> NameList) {
  std::vector<MatcherPair> Matchers;                                                                                         
  std::transform(                                                                                                            
      NameList.begin(), NameList.end(), std::back_inserter(Matchers),                                                        
      [](const std::string& Name) {                                                                                          
        return MatcherPair{                                                                                                  
            .regex = llvm::Regex(Name),                                                                                      
            .matchQualifiedName = Name.find("::") != std::string::npos,                                                      
        };                                                                                                                   
      });
  return matchesAnyListNameImpl(std::move(Matchers));
}

[flx]

clang-tools-extra/clang-tidy/utils/Matchers.h

74

Thanks for the snippet! I tried, but ran into the issue that llvm::Regex can only be moved, but AST_MATCHER_P uses copy construction of its param somewhere. I was able to work around it by using shared ptrs:

struct RegexAndScope {                                                                                                       
  llvm::Regex regex;                                                                                                         
  bool matchQualifiedName = false;                                                                                           
};                                                                                                                           
                                                                                                                             
AST_MATCHER_P(NamedDecl, matchesAnyListedNameImpl,                                                                           
              std::vector<std::shared_ptr<RegexAndScope>>, Matchers) {                                                       
  return llvm::any_of(                                                                                                       
      Matchers, [&Node](const std::shared_ptr<RegexAndScope>& Matcher) {                                                     
        if (Matcher->matchQualifiedName) {                                                                                   
          return Matcher->regex.match(Node.getQualifiedNameAsString());                                                      
        }                                                                                                                    
        return Matcher->regex.match(Node.getName());                                                                         
      });                                                                                                                    
}                                                                                                                            
                                                                                                                             
ast_matchers::internal::Matcher<NamedDecl> matchesAnyListedName(                                                             
    const std::vector<std::string>& NameList) {                                                                              
  std::vector<std::shared_ptr<RegexAndScope>> Matchers;                                                                      
  std::transform(NameList.begin(), NameList.end(), std::back_inserter(Matchers),                                             
                 [](const std::string& Name) {                                                                               
                   auto RAS = std::make_shared<RegexAndScope>();                                                             
                   RAS->regex = llvm::Regex(Name);                                                                           
                   RAS->matchQualifiedName =                                                                                 
                       Name.find("::") != std::string::npos;                                                                 
                   return RAS;                                                                                               
                 });                                                                                                         
  return matchesAnyListedNameImpl(std::move(Matchers));                                                                      
}

What do you think?

[ymandel]

clang-tools-extra/clang-tidy/utils/Matchers.h

74

That works for me, but let's see what other reviewers think. Personally, I'd just define the class directly at this point:

class MatchesAnyListedNameMatcher
    : public ast_matchers::internal::MatcherInterface<NamedDecl> {
 public:
  explicit MatchesAnyListedNameMatcher(llvm::ArrayRef<std::string> NameList) {
    std::transform(
        NameList.begin(), NameList.end(), std::back_inserter(Matchers),
        [](const std::string& Name) {
          return MatcherPair{
              .regex = llvm::Regex(Name),
              .matchQualifiedName = Name.find("::") != std::string::npos,
          };
        });
  }
  bool matches(
      const NamedDecl& Node, ast_matchers::internal::ASTMatchFinder* Finder,
      ast_matchers::internal::BoundNodesTreeBuilder* Builder) const override {
    return llvm::any_of(Matchers, [&Node](const MatcherPair& Matcher) {
      if (Matcher.matchQualifiedName) {
        return Matcher.regex.match(Node.getQualifiedNameAsString());
      }
      return Matcher.regex.match(Node.getName());
    });
  }

 private:
  std::vector<MatcherPair> Matchers;
};

inline ::clang::ast_matchers::internal::Matcher<Type> matchesAnyListedName(
    llvm::ArrayRef<std::string> NameList) {
  return ::clang::ast_matchers::internal::makeMatcher(
      new MatchesAnyListedNameMatcher(NameList))
}

[hokein]

clang-tools-extra/clang-tidy/utils/Matchers.h

51–52

worth some comments on this.

73–91

If we pass the name as a llvm::StringRef, we can use if (Name.contains("::")) which seems a bit clearer to me.

73–91

there is a tricky case where the :: is a prefix of the Name, e.g. a global symbol x, the Name is ::x, and getQualifiedNameAsString of symbol x is x (without the :: prefix), then the matcher will not find a match, but we expect this is a match right?

74

+1 the idea looks good to me.

[flx]

Applied changes suggested by ymandel, thanks!

[flx]

clang-tools-extra/clang-tidy/utils/Matchers.h

51–52

Done in its new location.

73–91

I agree that's a tricky case.

We could prepend "::" to getQualifiedNameAsString() and always match against it. Would this work?

Or we could do this only for regex names that have a leading "::". This would avoid extra string operations, and users could still match using "^foo::Bar$" to match ::foo::Bar.

[ymandel]

clang-tools-extra/clang-tidy/utils/Matchers.h

60

I don't think that the clang style-guide accepts designated initializers yet.

73–91

Or we could do this only for regex names that have a leading "::". This would avoid extra string operations, and users could still match using "^foo::Bar$" to match ::foo::Bar.

This should work most of the time. There could still be mistakes, like if a user spells the "::" in some odd way, like "[:][:]", but that seems quite unlikely in practice.

79

Worth a comment i think.

86

nit: s/NamedDecls/NamedDecl's/

[flx]

Create a NameMatcher class that handles matching against the best name variant
(unqualified, qualified, fully qualified) of the NamedDecl.

[flx]

Thanks for the comments, PTAL!

clang-tools-extra/clang-tidy/utils/Matchers.h

79

Done by adding comments to each match mode.

clang-tools-extra/clang-tidy/utils/Matchers.h

AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
   return referenceType(pointee(qualType(isConstQualified())));
 }
 
 // Returns QualType matcher for pointers to const.
 AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
   using namespace ast_matchers;
   return pointerType(pointee(qualType(isConstQualified())));
 }
 
-AST_MATCHER_P(NamedDecl, matchesAnyListedName, std::vector<std::string>,
-              NameList) {
-  return llvm::any_of(NameList, [&Node](const std::string &Name) {
-      return llvm::Regex(Name).match(Node.getName());
+class MatchesAnyListedNameMatcher
+    : public ast_matchers::internal::MatcherInterface<NamedDecl> {
+public:
+  explicit MatchesAnyListedNameMatcher(llvm::ArrayRef<std::string> NameList) {
+    std::transform(NameList.begin(), NameList.end(),
+                   std::back_inserter(RegexAndScopes),
+                   [](const llvm::StringRef Name) {
+                     return RegexAndScope{
+                         .Regex = llvm::Regex(Name),
+                         .MatchQualifiedName = Name.contains("::"),
+                     };
                    });
   }
+  bool matches(
+      const NamedDecl &Node, ast_matchers::internal::ASTMatchFinder *Finder,
+      ast_matchers::internal::BoundNodesTreeBuilder *Builder) const override {
+    return llvm::any_of(RegexAndScopes, [&Node](const RegexAndScope &RAS) {
+      if (RAS.MatchQualifiedName) {
+        return RAS.Regex.match(Node.getQualifiedNameAsString());
+      }
+      return RAS.Regex.match(Node.getName());
+    });
+  }
+
+private:
+  struct RegexAndScope {
+    llvm::Regex Regex;
+    bool MatchQualifiedName = false;
+  };
+
+  std::vector<RegexAndScope> RegexAndScopes;
+};
+
+// Returns a matcher that matches NamedDecls against a list of provided regular
+// expressions. If a regular expression contains '::' the NamedDecls qualified
+// name will be used for matching, otherwise its name will be used.
+inline ::clang::ast_matchers::internal::Matcher<NamedDecl>
+matchesAnyListedName(llvm::ArrayRef<std::string> NameList) {
+  return ::clang::ast_matchers::internal::makeMatcher(
+      new MatchesAnyListedNameMatcher(NameList));
+}
 
 } // namespace matchers
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H

clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst

.. option:: WarnOnAllAutoCopies
    When `true`, warns on any use of `auto` as the type of the range-based for
    loop variable. Default is `false`.
 
 .. option:: AllowedTypes
 
    A semicolon-separated list of names of types allowed to be copied in each
    iteration. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
    every type with suffix `Ref`, `ref`, `Reference` and `reference`. The default
-   is empty.
+   is empty. If a name in the list contains the sequence `::` it is matched
+   against the qualified typename (i.e. `namespace::Type`, otherwise it is
+   matched against only the type name (i.e. `Type`).

clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst

 
 Options
 -------
 
 .. option:: AllowedTypes
 
    A semicolon-separated list of names of types allowed to be initialized by
    copying. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
-   every type with suffix `Ref`, `ref`, `Reference` and `reference`. The
-   default is empty.
+   every type with suffix `Ref`, `ref`, `Reference` and `reference`. The default
+   is empty. If a name in the list contains the sequence `::` it is matched
+   against the qualified typename (i.e. `namespace::Type`, otherwise it is
+   matched against only the type name (i.e. `Type`).

clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-value-param.rst

.. option:: IncludeStyle
 
    A string specifying which include-style is used, `llvm` or `google`. Default
    is `llvm`.
 
 .. option:: AllowedTypes
 
    A semicolon-separated list of names of types allowed to be passed by value.
    Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches every type
-   with suffix `Ref`, `ref`, `Reference` and `reference`. The default is empty.
+   with suffix `Ref`, `ref`, `Reference` and `reference`. The default is
+   empty. If a name in the list contains the sequence `::` it is matched against
+   the qualified typename (i.e. `namespace::Type`, otherwise it is matched
+   against only the type name (i.e. `Type`).

clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy-allowed-types.cpp

 // RUN: %check_clang_tidy %s performance-for-range-copy %t -- \
-// RUN:     -config="{CheckOptions: [{key: performance-for-range-copy.AllowedTypes, value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$'}]}" \
+// RUN:     -config="{CheckOptions: [{key: performance-for-range-copy.AllowedTypes, value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$;fully::Qualified'}]}" \
 // RUN:     -- -fno-delayed-template-parsing
 
 template <typename T>
 struct Iterator {
   void operator++() {}
   const T& operator*() {
     static T* TT = new T();
     return *TT;

 };
 
 template <typename T> struct SomeComplexTemplate {
   ~SomeComplexTemplate();
 };
 
 typedef SomeComplexTemplate<int> NotTooComplexRef;
 
+namespace fully {
+
+struct Qualified {
+  ~Qualified();
+};
+
+} // namespace fully
+
 void negativeSmartPointer() {
   for (auto P : View<Iterator<SmartPointer>>()) {
     auto P2 = P;
   }
 }
 
 void negative_smart_pointer() {
   for (auto p : View<Iterator<smart_pointer>>()) {
void positiveOtherType() {
   }
 }
 
 void negativeNotTooComplexRef() {
   for (NotTooComplexRef R : View<Iterator<NotTooComplexRef>>()) {
     auto R2 = R;
   }
 }
+
+void negativeFullyQualified() {
+  for (auto Q : View<Iterator<fully::Qualified>>()) {
+    auto Q2 = Q;
+  }
+  using fully::Qualified;
+  for (auto Q : View<Iterator<Qualified>>()) {
+    auto Q2 = Q;
+  }
+}