[PATCH] Improve diagnostic message for misplaced square brackets

[Richard Smith]

+    if (Tok.is(tok::l_square)) {
+      //if (ParseMisplacedBracketDeclarator(D)) {
+      ParseMisplacedBracketDeclarator(D);
+      //  goto PastIdentifier;
+      return;

Remove comments here; maybe just

if (Tok.is(tok::l_square))
  return ParseMisplacedBracketDeclarator(D);


+  // Sometimes, parentheses are needed.  Determine if they are needed by
+  // looking at the current token.  If they are needed, store the location
+  // of the left parentheses in SuggestParenLoc.
+  SourceLocation SuggestParenLoc;
+  if (Tok.isNot(tok::identifier) && Tok.isNot(tok::l_paren) &&
+      Tok.isNot(tok::semi)) {
+    SuggestParenLoc = Tok.getLocation();
+    D.getName().EndLocation = SourceLocation();
+  }
+
+  // Now that the brackets are removed, try parsing the declarator again.
+  ParseDeclaratorInternal(D, &Parser::ParseDirectDeclarator);

This logic for determining whether parens are needed doesn't seem to cover
all cases. In particular:

  int [3] T::*p;

needs parens but starts with an identifier. Conversely,

  int [3] ::n;

doesn't need parens, but gets them. Instead, how about something like:

SourceLocation LocBeforeDeclarator = Tok.getLocation();
ParseDeclaratorInternal(D, &Parser::ParseDirectDeclarator);

bool NeedParens = false;
if (D.getNumTypeObjects()) {
  switch (D.getTypeObject(D.getNumTypeObjects())) {
    case DeclaratorChunk::Pointer:
    case DeclaratorChunk::Reference:
    case DeclaratorChunk::BlockPointer:
    case DeclaratorChunk::MemberPointer:
      NeedParens = true;
      break;
    case /* all the rest */
      break;
  }
}

// ...


+  // Adding back the bracket info to the end of the Declarator.

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.


+  // The missing identifier would have been diagnosed in
ParseDirectDeclarator.
+  // If parentheses are required, always suggest them.
+  if (!D.getIdentifier() && !SuggestParenLoc.isValid())
+    return;

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


+  SourceLocation EndBracketLoc =
+      TempDeclarator.getTypeObject(TempDeclarator.getNumTypeObjects() - 1)
+          .EndLoc;

Is this just 'TempDeclarator.getLocEnd()'?


+  // When suggesting parentheses, the closing paren should not be before
the
+  // opening paren.
+  if (SuggestParenLoc.isValid() && EndLoc < SuggestParenLoc)

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?

Also, you still need to produce some kind of diagnostic on this code path.


On Tue, Jun 10, 2014 at 6:49 PM, Richard Trieu <rtrieu at google.com> wrote:

> Clean up brackets.cpp test case
> Add new tests for reference variables
> Use ParseDeclaratorInternal instead of ParseDirectDeclarator
> Remove use of token stream mangling and backtracking
> Skip the error message when the closing paren comes before the opening
> paren in the fix-it hint
>
> 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"
>
> http://reviews.llvm.org/D2712
>
> Files:
>   include/clang/Basic/DiagnosticParseKinds.td
>   include/clang/Parse/Parser.h
>   lib/Parse/ParseDecl.cpp
>   test/Parser/brackets.c
>   test/Parser/brackets.cpp
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140611/30860404/attachment.html>