Re: r341141 - Extract parseBindID method

[Stephen Kelly via cfe-commits]

On 06/09/18 20:42, Roman Lebedev wrote:

You can specify the dependencies between the differentials in phabricator.



I didn't know that, thanks!
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r341141 - Extract parseBindID method

[Roman Lebedev via cfe-commits]

On Thu, Sep 6, 2018 at 10:05 PM, Stephen Kelly via cfe-commits
 wrote:
>
> Yes, this was a prerequisite to https://reviews.llvm.org/D51259. For some
> reason the reviewer accepted that one, but not this one. I simply assumed
> that was a mistake and committed this one.
I'm not sure this is how the review works in LLVM, in general.

> Perhaps part of the problem is that I do one thing per commit, and that
> confuses reviewers. Maybe I should squash things for review? I don't know.
You can specify the dependencies between the differentials in phabricator.

> Thanks for any post-commit review!
>
> Stephen.
Roman.

> On 31/08/18 07:06, Roman Lebedev via cfe-commits wrote:
>>
>> I don't think this was reviewed. The differential is not in 'accepted'
>> state.
>>
>> Roman.
>>
>> On Fri, Aug 31, 2018 at 2:11 AM, Stephen Kelly via cfe-commits
>>  wrote:
>>>
>>> Author: steveire
>>> Date: Thu Aug 30 16:11:01 2018
>>> New Revision: 341141
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=341141=rev
>>> Log:
>>> Extract parseBindID method
>>>
>>> Subscribers: cfe-commits
>>>
>>> Differential Revision: https://reviews.llvm.org/D51258
>>>
>>> Modified:
>>>  cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
>>>  cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
>>>
>>> Modified: cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h?rev=341141=341140=341141=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h (original)
>>> +++ cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h Thu Aug 30
>>> 16:11:01 2018
>>> @@ -234,6 +234,7 @@ private:
>>>const NamedValueMap *NamedValues,
>>>Diagnostics *Error);
>>>
>>> +  bool parseBindID(std::string );
>>> bool parseExpressionImpl(VariantValue *Value);
>>> bool parseMatcherExpressionImpl(const TokenInfo ,
>>> VariantValue *Value);
>>>
>>> Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp?rev=341141=341140=341141=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp (original)
>>> +++ cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp Thu Aug 30 16:11:01 2018
>>> @@ -359,6 +359,43 @@ bool Parser::parseIdentifierPrefixImpl(V
>>> return parseMatcherExpressionImpl(NameToken, Value);
>>>   }
>>>
>>> +bool Parser::parseBindID(std::string ) {
>>> +  // Parse .bind("foo")
>>> +  assert(Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period);
>>> +  Tokenizer->consumeNextToken(); // consume the period.
>>> +  const TokenInfo BindToken = Tokenizer->consumeNextToken();
>>> +  if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
>>> +addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
>>> +return false;
>>> +  }
>>> +
>>> +  const TokenInfo OpenToken = Tokenizer->consumeNextToken();
>>> +  const TokenInfo IDToken = Tokenizer->consumeNextToken();
>>> +  const TokenInfo CloseToken = Tokenizer->consumeNextToken();
>>> +
>>> +  // TODO: We could use different error codes for each/some to be more
>>> +  //   explicit about the syntax error.
>>> +  if (BindToken.Kind != TokenInfo::TK_Ident ||
>>> +  BindToken.Text != TokenInfo::ID_Bind) {
>>> +Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
>>> +return false;
>>> +  }
>>> +  if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
>>> +Error->addError(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
>>> +return false;
>>> +  }
>>> +  if (IDToken.Kind != TokenInfo::TK_Literal ||
>>> !IDToken.Value.isString()) {
>>> +Error->addError(IDToken.Range, Error->ET_ParserMalformedBindExpr);
>>> +return false;
>>> +  }
>>> +  if (CloseToken.Kind != TokenInfo::TK_CloseParen) {
>>> +Error->addError(CloseToken.Range,
>>> Error->ET_ParserMalformedBindExpr);
>>> +return false;
>>> +  }
>>> +  BindID = IDToken.Value.getString();
>>> +  return true;
>>> +}
>>> +
>>>   /// Parse and validate a matcher expression.
>>>   /// \return \c true on success, in which case \c Value has the matcher
>>> parsed.
>>>   ///   If the input is malformed, or some argument has an error, it
>>> @@ -425,38 +462,8 @@ bool Parser::parseMatcherExpressionImpl(
>>>
>>> std::string BindID;
>>> if (Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period) {
>>> -// Parse .bind("foo")
>>> -Tokenizer->consumeNextToken();  // consume the period.
>>> -const TokenInfo BindToken = Tokenizer->consumeNextToken();
>>> -if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
>>> -  addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
>>> +if (!parseBindID(BindID))
>>> return false;
>>> -}
>>> -
>>> -const 

Re: r341141 - Extract parseBindID method

[Stephen Kelly via cfe-commits]

Yes, this was a prerequisite to https://reviews.llvm.org/D51259. For 
some reason the reviewer accepted that one, but not this one. I simply 
assumed that was a mistake and committed this one.


Perhaps part of the problem is that I do one thing per commit, and that 
confuses reviewers. Maybe I should squash things for review? I don't know.


Thanks for any post-commit review!

Stephen.

On 31/08/18 07:06, Roman Lebedev via cfe-commits wrote:

I don't think this was reviewed. The differential is not in 'accepted' state.

Roman.

On Fri, Aug 31, 2018 at 2:11 AM, Stephen Kelly via cfe-commits
 wrote:

Author: steveire
Date: Thu Aug 30 16:11:01 2018
New Revision: 341141

URL: http://llvm.org/viewvc/llvm-project?rev=341141=rev
Log:
Extract parseBindID method

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D51258

Modified:
 cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
 cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp

Modified: cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h?rev=341141=341140=341141=diff
==
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h Thu Aug 30 16:11:01 
2018
@@ -234,6 +234,7 @@ private:
   const NamedValueMap *NamedValues,
   Diagnostics *Error);

+  bool parseBindID(std::string );
bool parseExpressionImpl(VariantValue *Value);
bool parseMatcherExpressionImpl(const TokenInfo ,
VariantValue *Value);

Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp?rev=341141=341140=341141=diff
==
--- cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp Thu Aug 30 16:11:01 2018
@@ -359,6 +359,43 @@ bool Parser::parseIdentifierPrefixImpl(V
return parseMatcherExpressionImpl(NameToken, Value);
  }

+bool Parser::parseBindID(std::string ) {
+  // Parse .bind("foo")
+  assert(Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period);
+  Tokenizer->consumeNextToken(); // consume the period.
+  const TokenInfo BindToken = Tokenizer->consumeNextToken();
+  if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
+addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
+return false;
+  }
+
+  const TokenInfo OpenToken = Tokenizer->consumeNextToken();
+  const TokenInfo IDToken = Tokenizer->consumeNextToken();
+  const TokenInfo CloseToken = Tokenizer->consumeNextToken();
+
+  // TODO: We could use different error codes for each/some to be more
+  //   explicit about the syntax error.
+  if (BindToken.Kind != TokenInfo::TK_Ident ||
+  BindToken.Text != TokenInfo::ID_Bind) {
+Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
+return false;
+  }
+  if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
+Error->addError(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
+return false;
+  }
+  if (IDToken.Kind != TokenInfo::TK_Literal || !IDToken.Value.isString()) {
+Error->addError(IDToken.Range, Error->ET_ParserMalformedBindExpr);
+return false;
+  }
+  if (CloseToken.Kind != TokenInfo::TK_CloseParen) {
+Error->addError(CloseToken.Range, Error->ET_ParserMalformedBindExpr);
+return false;
+  }
+  BindID = IDToken.Value.getString();
+  return true;
+}
+
  /// Parse and validate a matcher expression.
  /// \return \c true on success, in which case \c Value has the matcher parsed.
  ///   If the input is malformed, or some argument has an error, it
@@ -425,38 +462,8 @@ bool Parser::parseMatcherExpressionImpl(

std::string BindID;
if (Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period) {
-// Parse .bind("foo")
-Tokenizer->consumeNextToken();  // consume the period.
-const TokenInfo BindToken = Tokenizer->consumeNextToken();
-if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
-  addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
+if (!parseBindID(BindID))
return false;
-}
-
-const TokenInfo OpenToken = Tokenizer->consumeNextToken();
-const TokenInfo IDToken = Tokenizer->consumeNextToken();
-const TokenInfo CloseToken = Tokenizer->consumeNextToken();
-
-// TODO: We could use different error codes for each/some to be more
-//   explicit about the syntax error.
-if (BindToken.Kind != TokenInfo::TK_Ident ||
-BindToken.Text != TokenInfo::ID_Bind) {
-  Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
-  return false;
-}
-if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
-  Error->addError(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
-  

Re: r341141 - Extract parseBindID method

[Roman Lebedev via cfe-commits]

I don't think this was reviewed. The differential is not in 'accepted' state.

Roman.

On Fri, Aug 31, 2018 at 2:11 AM, Stephen Kelly via cfe-commits
 wrote:
> Author: steveire
> Date: Thu Aug 30 16:11:01 2018
> New Revision: 341141
>
> URL: http://llvm.org/viewvc/llvm-project?rev=341141=rev
> Log:
> Extract parseBindID method
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D51258
>
> Modified:
> cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
> cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
>
> Modified: cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h?rev=341141=341140=341141=diff
> ==
> --- cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h (original)
> +++ cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h Thu Aug 30 16:11:01 
> 2018
> @@ -234,6 +234,7 @@ private:
>   const NamedValueMap *NamedValues,
>   Diagnostics *Error);
>
> +  bool parseBindID(std::string );
>bool parseExpressionImpl(VariantValue *Value);
>bool parseMatcherExpressionImpl(const TokenInfo ,
>VariantValue *Value);
>
> Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp?rev=341141=341140=341141=diff
> ==
> --- cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp (original)
> +++ cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp Thu Aug 30 16:11:01 2018
> @@ -359,6 +359,43 @@ bool Parser::parseIdentifierPrefixImpl(V
>return parseMatcherExpressionImpl(NameToken, Value);
>  }
>
> +bool Parser::parseBindID(std::string ) {
> +  // Parse .bind("foo")
> +  assert(Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period);
> +  Tokenizer->consumeNextToken(); // consume the period.
> +  const TokenInfo BindToken = Tokenizer->consumeNextToken();
> +  if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
> +addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
> +return false;
> +  }
> +
> +  const TokenInfo OpenToken = Tokenizer->consumeNextToken();
> +  const TokenInfo IDToken = Tokenizer->consumeNextToken();
> +  const TokenInfo CloseToken = Tokenizer->consumeNextToken();
> +
> +  // TODO: We could use different error codes for each/some to be more
> +  //   explicit about the syntax error.
> +  if (BindToken.Kind != TokenInfo::TK_Ident ||
> +  BindToken.Text != TokenInfo::ID_Bind) {
> +Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
> +return false;
> +  }
> +  if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
> +Error->addError(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
> +return false;
> +  }
> +  if (IDToken.Kind != TokenInfo::TK_Literal || !IDToken.Value.isString()) {
> +Error->addError(IDToken.Range, Error->ET_ParserMalformedBindExpr);
> +return false;
> +  }
> +  if (CloseToken.Kind != TokenInfo::TK_CloseParen) {
> +Error->addError(CloseToken.Range, Error->ET_ParserMalformedBindExpr);
> +return false;
> +  }
> +  BindID = IDToken.Value.getString();
> +  return true;
> +}
> +
>  /// Parse and validate a matcher expression.
>  /// \return \c true on success, in which case \c Value has the matcher 
> parsed.
>  ///   If the input is malformed, or some argument has an error, it
> @@ -425,38 +462,8 @@ bool Parser::parseMatcherExpressionImpl(
>
>std::string BindID;
>if (Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period) {
> -// Parse .bind("foo")
> -Tokenizer->consumeNextToken();  // consume the period.
> -const TokenInfo BindToken = Tokenizer->consumeNextToken();
> -if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
> -  addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
> +if (!parseBindID(BindID))
>return false;
> -}
> -
> -const TokenInfo OpenToken = Tokenizer->consumeNextToken();
> -const TokenInfo IDToken = Tokenizer->consumeNextToken();
> -const TokenInfo CloseToken = Tokenizer->consumeNextToken();
> -
> -// TODO: We could use different error codes for each/some to be more
> -//   explicit about the syntax error.
> -if (BindToken.Kind != TokenInfo::TK_Ident ||
> -BindToken.Text != TokenInfo::ID_Bind) {
> -  Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
> -  return false;
> -}
> -if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
> -  Error->addError(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
> -  return false;
> -}
> -if (IDToken.Kind != TokenInfo::TK_Literal || !IDToken.Value.isString()) {
> -  Error->addError(IDToken.Range, Error->ET_ParserMalformedBindExpr);
> -  return false;
> -}
> -if (CloseToken.Kind !=