[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,
----------------
njames93 wrote:
> aaron.ballman wrote:
> > 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?
> It has to be shared ownership for the polymorphic matchers to enable reuse of `PolymorphicMatcherWithParam1`.  When a matcher or type `T` is required from a `PolymorphicMatcherWithParam1` it creates a copy of the Param. As you can't copy from a `unique_ptr`, a `shared_ptr` makes sense.
Drat, but reasonable, thanks for the explanation.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.cpp:126
+      Flag |= llvm::Regex::IgnoreCase;
+    else if (OrFlag == "NewLine")
+      Flag |= llvm::Regex::Newline;
----------------
njames93 wrote:
> aaron.ballman wrote:
> > 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?
> That could work, or I implement the `ArgTypeTraits::getBestGuess` to handle small issues like that. Would be a little trickier than the other cases as It would need to support `|`. WDYT?
Hmm, I don't have a firm opinion but am slightly leaning towards implementing `getBestGuess` and using the more strict spellings based purely on personal preference.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:745
+  bool isVariadic() const override { return true; }
+  unsigned getNumArgs() const override { return 0; }
+
----------------
njames93 wrote:
> aaron.ballman wrote:
> > I may be misunderstanding this, but it seems suspicious that `getNumArgs()` returns 0 but `getArgKinds()` suggests at least one argument. Is that intentional?
> It was my understanding that when `isVariadic()` returns true, `getNumArgs()` is not used. I could change it to return 1, but that wouldn't be any more correct than it is now, nor would it have any functional change.
Ah, so the real issue is that `getNumArgs()` is pure virtual, which forces us to ask these sort of silly questions. Good to know, I'm fine with `0`, I was just surprised.


================
Comment at: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp:379
+      ParseMatcherWithError(
+          R"query(namedDecl(matchesName("[ABC]*", "IgnoreCase | Basicregex")))query"));
 }
----------------
njames93 wrote:
> aaron.ballman wrote:
> > Should we have a failure for something like `IgnoreCase | NoFlags` ?
> I'm against that, its perfectly legal in non dynamic matchers land to write `matchesName("[ABC]*", IgnoreCase | NoFlags)` Even if it is a little redundant.
Okay, fine by me. I only brought it up because it seems like a very confused user and we have the opportunity to diagnose (due to the dynamic nature here as opposed to the static nature of C++ using bitwise OR directly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82706