[PATCH] D82706: [ASTMatchers] Enhanced support for matchers taking Regex arguments

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1952
 
+std::shared_ptr<llvm::Regex> createAndVerifyRegex(StringRef Regex,
+                                                  llvm::Regex::RegexFlags Flags,
----------------
Is there a reason this needs to be a shared pointer as opposed to a unique pointer that gets moved into the class owning it?


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.cpp:126
+      Flag |= llvm::Regex::IgnoreCase;
+    else if (OrFlag == "NewLine")
+      Flag |= llvm::Regex::Newline;
----------------
This makes me think we want to do a case insensitive comparison rather than an exact case match. Personally, I would always write `NewLine` as the string literal, but I could easily see someone writing `Newline` to match the spelling of the RegEx flag and being surprised it doesn't work. WDYT?


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:745
+  bool isVariadic() const override { return true; }
+  unsigned getNumArgs() const override { return 0; }
+
----------------
I may be misunderstanding this, but it seems suspicious that `getNumArgs()` returns 0 but `getArgKinds()` suggests at least one argument. Is that intentional?


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:770
+          << Args[0].Value.getTypeAsString();
+    }
+    if (Args.size() == 1) {
----------------
Are you missing a `return` here, after issuing the error?


================
Comment at: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp:379
+      ParseMatcherWithError(
+          R"query(namedDecl(matchesName("[ABC]*", "IgnoreCase | Basicregex")))query"));
 }
----------------
Should we have a failure for something like `IgnoreCase | NoFlags` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82706