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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits