njames93 marked 7 inline comments as done.
njames93 added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1952
+std::shared_ptr<llvm::Regex> createAndVerifyRegex(StringRef Regex,
+ llvm::Regex::RegexFlags
Flags,
----------------
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.
================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.cpp:126
+ Flag |= llvm::Regex::IgnoreCase;
+ else if (OrFlag == "NewLine")
+ Flag |= llvm::Regex::Newline;
----------------
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?
================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:745
+ bool isVariadic() const override { return true; }
+ unsigned getNumArgs() const override { return 0; }
+
----------------
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.
================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:770
+ << Args[0].Value.getTypeAsString();
+ }
+ if (Args.size() == 1) {
----------------
aaron.ballman wrote:
> Are you missing a `return` here, after issuing the error?
Yes I am
================
Comment at: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp:379
+ ParseMatcherWithError(
+ R"query(namedDecl(matchesName("[ABC]*", "IgnoreCase |
Basicregex")))query"));
}
----------------
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.
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
