<div dir="ltr"><div>+    if (Tok.is(tok::l_square)) {</div><div>+      //if (ParseMisplacedBracketDeclarator(D)) {</div><div>+      ParseMisplacedBracketDeclarator(D);</div><div>+      //  goto PastIdentifier;</div><div>+      return;</div>
<div><br></div><div>Remove comments here; maybe just</div><div><br></div><div>if (Tok.is(tok::l_square))</div><div>  return ParseMisplacedBracketDeclarator(D);</div><div><br></div><div><br></div><div><div>+  // Sometimes, parentheses are needed.  Determine if they are needed by</div>
<div>+  // looking at the current token.  If they are needed, store the location</div><div>+  // of the left parentheses in SuggestParenLoc.</div><div>+  SourceLocation SuggestParenLoc;</div><div>+  if (Tok.isNot(tok::identifier) && Tok.isNot(tok::l_paren) &&</div>
<div>+      Tok.isNot(tok::semi)) {</div><div>+    SuggestParenLoc = Tok.getLocation();</div><div>+    D.getName().EndLocation = SourceLocation();</div><div>+  }</div><div>+</div><div>+  // Now that the brackets are removed, try parsing the declarator again.</div>
<div>+  ParseDeclaratorInternal(D, &Parser::ParseDirectDeclarator);</div></div><div><br></div><div>This logic for determining whether parens are needed doesn't seem to cover all cases. In particular:</div><div><br>
</div><div>  int [3] T::*p;</div><div><br></div><div>needs parens but starts with an identifier. Conversely,</div><div><br></div><div>  int [3] ::n;</div><div><br></div><div>doesn't need parens, but gets them. Instead, how about something like:</div>
<div><br></div><div>SourceLocation LocBeforeDeclarator = Tok.getLocation();</div><div>ParseDeclaratorInternal(D, &Parser::ParseDirectDeclarator);</div><div><br></div><div>bool NeedParens = false;</div><div>if (D.getNumTypeObjects()) {</div>
<div>  switch (D.getTypeObject(D.getNumTypeObjects())) {</div><div>    case DeclaratorChunk::Pointer:</div><div>    case DeclaratorChunk::Reference:</div><div>    case DeclaratorChunk::BlockPointer:</div><div>    case DeclaratorChunk::MemberPointer:</div>
<div>      NeedParens = true;</div><div>      break;</div><div>    case /* all the rest */</div><div>      break;</div><div>  }</div><div>}</div><div><br></div><div>// ...</div><div><br></div><div><br></div><div>+  // Adding back the bracket info to the end of the Declarator.<br>
</div><div><br></div><div>If you've detected that you need parens, it'd be nice to fabricate a DeclaratorChunk for the parens, just in case something downstream is making assumptions about what DeclaratorChunks can exist based on how types can be written.</div>
<div><br></div><div><br></div><div><div>+  // The missing identifier would have been diagnosed in ParseDirectDeclarator.</div><div>+  // If parentheses are required, always suggest them.</div><div>+  if (!D.getIdentifier() && !SuggestParenLoc.isValid())</div>
<div>+    return;</div></div><div><br></div><div>It would be nice to assert !D.mayOmitIdentifier() somewhere in this function, maybe right next to this check, since you're relying on that here. (I could certainly imagine this code getting reused for parsing abstract declarators at some point.)</div>
<div><br></div><div><br></div><div><div>+  SourceLocation EndBracketLoc =</div><div>+      TempDeclarator.getTypeObject(TempDeclarator.getNumTypeObjects() - 1)</div><div>+          .EndLoc;</div></div><div><br></div><div>
Is this just 'TempDeclarator.getLocEnd()'?</div><div><br></div><div><br></div><div><div>+  // When suggesting parentheses, the closing paren should not be before the</div><div>+  // opening paren.</div><div>+  if (SuggestParenLoc.isValid() && EndLoc < SuggestParenLoc)</div>
</div><div><br></div><div>This kind of SourceLocation comparison won't work in general (for instance, it will do weird things if the two locations have different FileIDs). Does this error case really happen?</div><div>
<br></div><div>Also, you still need to produce some kind of diagnostic on this code path.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 10, 2014 at 6:49 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Clean up brackets.cpp test case<br>
Add new tests for reference variables<br>
Use ParseDeclaratorInternal instead of ParseDirectDeclarator<br>
Remove use of token stream mangling and backtracking<br>
Skip the error message when the closing paren comes before the opening paren in the fix-it hint<br>
<br>
The main difference is restarting the parse earlier with ParseDeclaratorInternal, which allows for parsing the leading '*' and '&' tokens in "int [1] *foo" and "int [2] &bar"<br>

<div class="HOEnZb"><div class="h5"><br>
<a href="http://reviews.llvm.org/D2712" target="_blank">http://reviews.llvm.org/D2712</a><br>
<br>
Files:<br>
  include/clang/Basic/DiagnosticParseKinds.td<br>
  include/clang/Parse/Parser.h<br>
  lib/Parse/ParseDecl.cpp<br>
  test/Parser/brackets.c<br>
  test/Parser/brackets.cpp<br>
</div></div></blockquote></div><br></div>