[PATCH] Improve diagnostic message for misplaced square brackets

[Richard Trieu]

>>! In D2712#21, @rsmith wrote:
> +    if (Tok.is(tok::l_square)) {
> +      //if (ParseMisplacedBracketDeclarator(D)) {
> +      ParseMisplacedBracketDeclarator(D);
> +      //  goto PastIdentifier;
> +      return;
> 
> Remove comments here; maybe just
> 
Removed comments.

>   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;
>     }
>   }
> 
Paren adding updated.
> 
> 
> +  // 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.
> 
Done.  New paren DeclaratorChuck added right after figuring out they are needed.
> 
> +  // 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.)
> 
!D.mayOmitIdentifier() asserted at the beginning of the function since this function assumed the existence of an identifier.
> 
> +  SourceLocation EndBracketLoc =
> +      TempDeclarator.getTypeObject(TempDeclarator.getNumTypeObjects() - 1)
> +          .EndLoc;
> 
> Is this just 'TempDeclarator.getLocEnd()'?
> 
Yes.  Changed.
> 
> +  // 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?
> 
Changed it to detect that the Declarator end location changes after the call to ParseDeclaratorInternal.  Something like "int [];" will trigger this case.
> Also, you still need to produce some kind of diagnostic on this code path.
The ParseDeclaratorInternal should provide the error here.

http://reviews.llvm.org/D2712