NFC: Port QueryParser to StringRef

[steveire]

Summary

There is no reason for it to not be a StringRef. Making it one
simplifies existing code, and makes follow-up features easier.

[kristina]

LGTM aside from one comment. There's a few style nits however, and I think the lambda on one line could use a few newlines for readability.

clang-query/QueryParser.cpp

36

I don't think this is the best way of handling it, in fact I'm pretty certain you're leaking memory here.

[steveire]

Run clang-format

[steveire]

Thanks, I ran clang-format, and I'd rather let it be the rulemaker, rather than try to add newlines in the lambdas :).

clang-query/QueryParser.cpp

36

Hmm, I didn't know we could get a mem leak with StringRef. Can you explain?

[aaron.ballman]

clang-query/QueryParser.cpp

35

Why not just return Line?

36

There's no memory leak here that I can see -- StringRef is non-owning.

37

Why not just return Line here as well?

40–42

Can you use Line.take_until(isWhitespace);?

[steveire]

clang-query/QueryParser.cpp

35

It is the pre-existing behavior to return a zero-length StringRef with a valid Begin pointer in this case. I think the reason is something to do with code completion. I can check later.

37

That's pre-existing. Seems more clear to leave it as is.

[aaron.ballman]

clang-query/QueryParser.cpp

35

I believe returning Line is identical -- you should wind up with the same "begin" pointer for the returned string and a length of zero (because Line must be empty by this point).

37

I don't have a strong preference, but I raised the point because it seems odd to explicitly clear a StringRef and then return a new, different, empty StringRef on the next line.

[steveire]

clang-query/QueryParser.cpp

35

Actually, I can see it by reading the code - P->CompletionPos <= Word.data() in the LexOrCompleteWord ctor woudln't work anymore without this hack. (Working on removing the hack is out of scope of this change).

[steveire]

clang-query/QueryParser.cpp

35

Ok, I see your new comment now. I guess that will work. I don't know the behavior of ltrim and whether it preserves that property.

[steveire]

clang-query/QueryParser.cpp

37

Well, code changes, and Line may not always be empty here. As it is, this communicates that we're returning an empty StringRef and always will (until this line has a reason to change).

[aaron.ballman]

clang-query/QueryParser.cpp

35

StringRefs are immutable and non-owning, so ltrim() returns Line.begin + someOffset in essence. That should preserve the behavior you need here.

37

Okay, that's good enough for me, thanks!

[aaron.ballman]

LGTM aside from the nits pointed out; you can fix and commit without another round of review.

clang-query/QueryParser.h

public:
   /// likely to least likely.
   ///
   /// \return A vector of completions for \a Line.
   static std::vector<llvm::LineEditor::Completion>
   complete(StringRef Line, size_t Pos, const QuerySession &QS);
 
 private:
   QueryParser(StringRef Line, const QuerySession &QS)
-      : Begin(Line.begin()), End(Line.end()), CompletionPos(nullptr), QS(QS) {}
+      : Line(Line), CompletionPos(nullptr), QS(QS) {}
 
   StringRef lexWord();
 
   template <typename T> struct LexOrCompleteWord;
 
   QueryRef parseSetBool(bool QuerySession::*Var);
   template <typename QueryType> QueryRef parseSetOutputKind();
   QueryRef completeMatcherExpression();
 
   QueryRef endQuery(QueryRef Q);
 
   /// \brief Parse [\p Begin,\p End).
   ///
   /// \return A reference to the parsed query object, which may be an
   /// \c InvalidQuery if a parse error occurs.
   QueryRef doParse();
 
-  const char *Begin;
-  const char *End;
+  StringRef Line;
 
   const char *CompletionPos;
   std::vector<llvm::LineEditor::Completion> Completions;
 
   const QuerySession &QS;
 };
 
 } // namespace query
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_QUERY_QUERY_PARSER_H

clang-query/QueryParser.cpp

 namespace clang {
 namespace query {
 
 // Lex any amount of whitespace followed by a "word" (any sequence of
 // non-whitespace characters) from the start of region [Begin,End).  If no word
 // is found before End, return StringRef().  Begin is adjusted to exclude the
 // lexed region.
 StringRef QueryParser::lexWord() {
-  while (true) {
-    if (Begin == End)
-      return StringRef(Begin, 0);
+  Line = Line.ltrim();
  
-    if (!isWhitespace(*Begin))
-      break;
-
-    ++Begin;
-  }
+  if (Line.empty())
+    return StringRef(Line.begin(), 0);
 
-  if (*Begin == '#') {
-    End = Begin;
+  if (Line.front() == '#') {
+    Line = {};
     return StringRef();
   }
 
-  const char *WordBegin = Begin;
-
-  while (true) {
-    ++Begin;
-
-    if (Begin == End || isWhitespace(*Begin))
-      return StringRef(WordBegin, Begin - WordBegin);
-  }
+  StringRef Word = Line.take_while([](char c){ return !isWhitespace(c); });
+  Line = Line.drop_front(Word.size());
+  return Word;
 }
 
 // This is the StringSwitch-alike used by lexOrCompleteWord below. See that
 // function for details.
 template <typename T> struct QueryParser::LexOrCompleteWord {
   StringRef Word;
   StringSwitch<T> Switch;
 
template <typename QueryType> QueryRef QueryParser::parseSetOutputKind() {
   case OK_Print:
     return new QueryType(&QuerySession::PrintOutput);
   }
 
   llvm_unreachable("Invalid output kind");
 }
 
 QueryRef QueryParser::endQuery(QueryRef Q) {
-  const char *Extra = Begin;
+  const StringRef Extra = Line;
   if (!lexWord().empty())
-    return new InvalidQuery("unexpected extra input: '" +
-                            StringRef(Extra, End - Extra) + "'");
+    return new InvalidQuery("unexpected extra input: '" + Extra + "'");
   return Q;
 }
 
 namespace {
 
 enum ParsedQueryKind {
   PQK_Invalid,
   PQK_Comment,
QueryRef makeInvalidQueryFromDiagnostics(const Diagnostics &Diag) {
   Diag.printToStreamFull(OS);
   return new InvalidQuery(OS.str());
 }
 
 } // namespace
 
 QueryRef QueryParser::completeMatcherExpression() {
   std::vector<MatcherCompletion> Comps = Parser::completeExpression(
-      StringRef(Begin, End - Begin), CompletionPos - Begin, nullptr,
+      Line, CompletionPos - Line.begin(), nullptr,
       &QS.NamedValues);
   for (auto I = Comps.begin(), E = Comps.end(); I != E; ++I) {
     Completions.push_back(LineEditor::Completion(I->TypedText, I->MatcherDecl));
   }
   return QueryRef();
 }
 
 QueryRef QueryParser::doParse() {
case PQK_Let: {
     if (Name.empty())
       return new InvalidQuery("expected variable name");
 
     if (CompletionPos)
       return completeMatcherExpression();
 
     Diagnostics Diag;
     ast_matchers::dynamic::VariantValue Value;
-    if (!Parser::parseExpression(StringRef(Begin, End - Begin), nullptr,
+    if (!Parser::parseExpression(Line, nullptr,
                                  &QS.NamedValues, &Value, &Diag)) {
       return makeInvalidQueryFromDiagnostics(Diag);
     }
 
     return new LetQuery(Name, Value);
   }
 
   case PQK_Match: {
     if (CompletionPos)
       return completeMatcherExpression();
 
     Diagnostics Diag;
-    auto MatcherSource = StringRef(Begin, End - Begin).trim();
+    auto MatcherSource = Line.trim();
     Optional<DynTypedMatcher> Matcher = Parser::parseMatcherExpression(
         MatcherSource, nullptr, &QS.NamedValues, &Diag);
     if (!Matcher) {
       return makeInvalidQueryFromDiagnostics(Diag);
     }
     return new MatchQuery(MatcherSource, *Matcher);
   }