Page MenuHomePhabricator

Allow newlines in AST Matchers in clang-query files
ClosedPublic

Authored by steveire on Dec 23 2019, 9:00 AM.

Diff Detail

Event Timeline

steveire created this revision.Dec 23 2019, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 9:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Dec 24 2019, 10:14 AM
clang-tools-extra/clang-query/Query.cpp
103–104

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).

106

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).

111

MaxLength and perhaps this type should be unsigned or size_t rather than int to avoid the unnecessary type conversion?

clang-tools-extra/clang-query/QueryParser.cpp
30

Comment missing a trailing full stop.

132

Definitely prefer using StringRef here -- I had no idea drop_while() returned this.

140

auto -> StringRef

243–246

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.

clang/lib/ASTMatchers/Dynamic/Parser.cpp
301

Add a full stop to the comment.

361

Spurious semi-colon?

steveire marked 9 inline comments as done.Dec 26 2019, 9:09 AM
steveire added inline comments.
clang-tools-extra/clang-query/Query.cpp
111

This is where auto makes most sense, but I made it unsigned instead.

steveire marked 2 inline comments as done.Dec 26 2019, 9:44 AM
steveire added inline comments.
clang-tools-extra/clang-query/QueryParser.cpp
243–246

I think it is set after the fact because it is set in endQuery().

aaron.ballman accepted this revision.Dec 26 2019, 9:51 AM

LGTM aside from some auto -> auto * nits.

clang-tools-extra/clang-query/QueryParser.cpp
243–246

Okay, that's reasonable. Thanks! Can you switch to auto * for these cases?

This revision is now accepted and ready to land.Dec 26 2019, 9:51 AM
This revision was automatically updated to reflect the committed changes.
gribozavr2 added inline comments.
clang/include/clang/ASTMatchers/Dynamic/Parser.h
167

Sorry for the late review comment, but I'd really appreciate if you could update the doc comments to explain why MatcherCode is taken by reference, and how it is mutated.

ymandel added inline comments.
clang/include/clang/ASTMatchers/Dynamic/Parser.h
167

+1 This broke our build because were passing a temporary (foo.trim()). An explanation at least would be appreciated.