aaron.ballman added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/Dynamic/Registry.h:36-37
+/// A smart (owning) pointer for MatcherDescriptor
+/// We can't use unique_ptr because MatcherDescriptor is forward declared
+class MatcherDescriptorPtr {
----------------
================
Comment at: clang/include/clang/ASTMatchers/Dynamic/Registry.h:40
+public:
+ MatcherDescriptorPtr(MatcherDescriptor * = nullptr);
+ ~MatcherDescriptorPtr();
----------------
I think it'd be cleaner to make callers be explicit about forming the smart
pointer.
================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:586-587
+ std::unique_ptr<MatcherDescriptor>
+ buildMatcherCtor(SourceRange NameRange, ArrayRef<ParserValue> Args,
+ Diagnostics *Error) const override {
+ return {};
----------------
Might as well make it obvious the function doesn't care about its arguments
(same elsewhere in the patch).
================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:1081-1083
+ if (NodeKinds.empty()) {
+ return {};
+ }
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:372
std::string BindID;
- if (!parseBindID(BindID))
+ Tokenizer->consumeNextToken(); // consume the period.
+ const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:373
+ Tokenizer->consumeNextToken(); // consume the period.
+ const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
+ if (ChainCallToken.Kind == TokenInfo::TK_CodeCompletion) {
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:435
+ assert(NameToken.Kind == TokenInfo::TK_Ident);
+ const TokenInfo OpenToken = Tokenizer->consumeNextToken();
+ if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:449
+bool Parser::parseBindID(TokenInfo BindToken, std::string &BindID) {
// Parse .bind("foo")
const TokenInfo OpenToken = Tokenizer->consumeNextToken();
----------------
The comment is a bit misleading in that this doesn't parse the `.bind` part,
only the `("foo")` bits.
Also, why does the function now accept `BindToken` but not use it?
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:491
+ // We must find a , token to continue.
+ const TokenInfo CommaToken = Tokenizer->consumeNextToken();
+ if (CommaToken.Kind != TokenInfo::TK_Comma) {
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:510
+
+ const auto NodeMatcherToken = Tokenizer->consumeNextToken();
+
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:521
+
+ auto MappedMatcher = S->lookupMatcherCtor(ArgValue.Text);
+
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:553
+
+ auto BuiltCtor = S->buildMatcherCtor(Ctor, NameToken.Range, Args, Error);
+
----------------
unique_ptr? or is this our custom smart pointer? Either way, please spell the
type out.
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:563
+ if (Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period) {
+ Tokenizer->consumeNextToken(); // consume the period.
+ const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:638-641
+ if (Ctor && *Ctor && S->isBuilderMatcher(*Ctor)) {
+ return parseMatcherBuilder(*Ctor, NameToken, OpenToken, Value);
+ }
+
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:691
+ Tokenizer->consumeNextToken();
+ const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
+ if (ChainCallToken.Kind == TokenInfo::TK_CodeCompletion) {
----------------
================
Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:580
+ ArrayRef<ParserValue> Args, Diagnostics *Error) {
+ return Ctor->buildMatcherCtor(NameRange, Args, Error).release();
+}
----------------
I think that having the explicit ctor call here would make it more obvious that
the `.release()` is being picked up by another smart pointer type.
================
Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:697
- if (IsPolymorphic) {
- OS << "Matcher<T> " << Name << "(Matcher<T>";
+ std::string TypedText = std::string(Name);
+
----------------
Any reason this declaration needed to move?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94880/new/
https://reviews.llvm.org/D94880
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
