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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to