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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:60
+                     return RegexAndScope{
+                         .Regex = llvm::Regex(Name),
+                         .MatchQualifiedName = Name.contains("::"),
----------------
I don't think that the clang style-guide accepts designated initializers yet.


================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:79
+    llvm::Regex Regex;
+    bool MatchQualifiedName = false;
+  };
----------------
Worth a comment i think.


================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:86
+// 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.
----------------
nit: s/NamedDecls/`NamedDecl`'s/


================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:55
   return llvm::any_of(NameList, [&Node](const std::string &Name) {
-      return llvm::Regex(Name).match(Node.getName());
-    });
+    if (Name.find("::") != std::string::npos) {
+      return llvm::Regex(Name).match(Node.getQualifiedNameAsString());
----------------
flx wrote:
> hokein wrote:
> > hokein wrote:
> > > If we pass the name as a `llvm::StringRef`, we can use `if (Name.contains("::"))` which seems a bit clearer to me.
> > 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?
> > 
> 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.
>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.





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98738/new/

https://reviews.llvm.org/D98738