aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-query/Query.cpp:106 + Source.split(Lines, "\n"); + auto firstLine = Lines[0]; + Lines.erase(Lines.begin(), Lines.begin() + 1); ---------------- `firstLine` should be `FirstLine` and I have a slight preference for `StringRef` instead of `auto` (but don't insist because `Lines` is so nearby and clear). ================ Comment at: clang-tools-extra/clang-query/Query.cpp:111 + } + int maxLength = firstLine.size(); std::string prefixText = "Matcher: "; ---------------- `MaxLength` and perhaps this type should be `unsigned` or `size_t` rather than `int` to avoid the unnecessary type conversion? ================ Comment at: clang-tools-extra/clang-query/Query.cpp:112 + int maxLength = firstLine.size(); std::string prefixText = "Matcher: "; + OS << "\n " << prefixText << firstLine; ---------------- Oh, I see the existing text is already inconsistent with naming conventions. You can ignore the renaming comments if you want -- we can always fix up the names post-commit -- or you can fix up this name (which is the only oddball in the file). ================ Comment at: clang-tools-extra/clang-query/QueryParser.cpp:30 + Line = Line.drop_while([this](char c) { + // Don't trim newlines + return StringRef(" \t\v\f\r").contains(c); ---------------- Comment missing a trailing full stop. ================ Comment at: clang-tools-extra/clang-query/QueryParser.cpp:132 + StringRef Extra = Line; + auto ExtraTrimmed = Extra.drop_while( + [](char c) { return StringRef(" \t\v\f\r").contains(c); }); ---------------- Definitely prefer using `StringRef` here -- I had no idea `drop_while()` returned `this`. ================ Comment at: clang-tools-extra/clang-query/QueryParser.cpp:140 + else { + auto TrailingWord = lexWord(); + if (TrailingWord.front() == '#') { ---------------- `auto` -> `StringRef` ================ Comment at: clang-tools-extra/clang-query/QueryParser.cpp:243-244 - return new LetQuery(Name, Value); + auto Q = new LetQuery(Name, Value); + Q->RemainingContent = Line; + return Q; ---------------- Would it make more sense to have `Query` with another constructor to hold the remaining content, and then thread that through `LetQuery`, `MatchQuery`, etc? Whether there is remaining content or not seems like a pretty important property of the query, so it seems reasonable to let people construct with that information rather than set it after the fact. If not, these should be `auto *` rather than deducing the pointer. ================ Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:301 + Code = Code.drop_while([this](char c) { + // Don't trim newlines + return StringRef(" \t\v\f\r").contains(c); ---------------- Add a full stop to the comment. ================ Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:361 + << "NewLine"; + ; + return false; ---------------- Spurious semi-colon? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71842/new/ https://reviews.llvm.org/D71842 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits