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