[cfe-commits] Note and FixIt for additional case of vexing parse

[Richard Smith]

> Index: include/clang/Basic/DiagnosticParseKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticParseKinds.td (revision 160664)
> +++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
> @@ -412,14 +412,11 @@
>  def err_expected_init_in_condition : Error<
>    "variable declaration in condition must have an initializer">;
>  def err_expected_init_in_condition_lparen : Error<
> -  "variable declaration in condition cannot have a parenthesized
initializer">;
> -def err_extraneous_rparen_in_condition : Error<
> -  "extraneous ')' after condition, expected a statement">;
> -def warn_parens_disambiguated_as_function_decl : Warning<
> -  "parentheses were disambiguated as a function declarator">,
> -  InGroup<VexingParse>;
> -def warn_dangling_else : Warning<
> -  "add explicit braces to avoid dangling else">,
> +  "variable declaration in condition cannot have a parenthesized
initializer">;
> +def err_extraneous_rparen_in_condition : Error<
> +  "extraneous ')' after condition, expected a statement">;
> +def warn_dangling_else : Warning<
> +  "add explicit braces to avoid dangling else">,

I'm not sure how it's happened, but your patch is changing these lines from
Unix to DOS line endings.

>    InGroup<DanglingElse>;
>  def err_expected_member_or_base_name : Error<
>    "expected class member or base class name">;
> Index: lib/Parse/ParseDecl.cpp
> ===================================================================
> --- lib/Parse/ParseDecl.cpp (revision 160664)
> +++ lib/Parse/ParseDecl.cpp (working copy)
> @@ -4317,17 +4317,14 @@
>        // The paren may be part of a C++ direct initializer, eg. "int
x(1);".
>        // In such a case, check if we actually have a function
declarator; if it
>        // is not, the declarator has been fully parsed.
> -      if (getLangOpts().CPlusPlus && D.mayBeFollowedByCXXDirectInit()) {
> -        // When not in file scope, warn for ambiguous function
declarators, just
> -        // in case the author intended it as a variable definition.
> -        bool warnIfAmbiguous = D.getContext() != Declarator::FileContext;
> -        if (!isCXXFunctionDeclarator(warnIfAmbiguous))
> +      bool IsAmbiguous = false;
> +      if (getLangOpts().CPlusPlus && D.mayBeFollowedByCXXDirectInit() &&
> +          !isCXXFunctionDeclarator(&IsAmbiguous))
>            break;

Please dedent this 'break;' by 2 spaces.

> -      }
>        ParsedAttributes attrs(AttrFactory);
>        BalancedDelimiterTracker T(*this, tok::l_paren);
>        T.consumeOpen();
> -      ParseFunctionDeclarator(D, attrs, T);
> +      ParseFunctionDeclarator(D, attrs, T, IsAmbiguous);
>        PrototypeScope.Exit();
>      } else if (Tok.is(tok::l_square)) {
>        ParseBracketDeclarator(D);
> Index: lib/Sema/SemaType.cpp
> ===================================================================
> --- lib/Sema/SemaType.cpp (revision 160664)
> +++ lib/Sema/SemaType.cpp (working copy)
> @@ -2418,6 +2419,104 @@
>          T = Context.getFunctionType(T, ArgTys.data(), ArgTys.size(),
EPI);
>        }
>
> +      // If we see "T var();" at block scope, where T is a class type,
it is
> +      // probably an attempt to initialize a variable, not a function
> +      // declaration.
> +      // We don't catch this case earlier, since there is no ambiguity
here.
> +      bool Pointer = false;
> +      bool FunctionDeclarator = false;
> +      for (unsigned i = 0, e = D.getNumTypeObjects(); i != e; ++i) {
> +        DeclaratorChunk &Chunk = D.getTypeObject(i);
> +        if (Chunk.Kind == DeclaratorChunk::Function) {
> +          FunctionDeclarator = true;
> +          break;
> +        }
> +        else if (Chunk.Kind == DeclaratorChunk::Pointer) {
> +          Pointer = true;
> +          break;
> +        }
> +        else if (Chunk.Kind == DeclaratorChunk::Paren)
> +          continue;
> +        else
> +          break;
> +      }

Please move this into an "if (FTI.isAmbiguous)" check. We don't want to pay
for this when building every function type. Also, if you can factor this
out to a separate function, that would help avoid disrupting the flow of
this function.

> +
> +      QualType RT = T->getAs<FunctionType>()->getResultType();
> +      if (!RT->isVoidType() && (!RT->isReferenceType() || FTI.NumArgs ==
1) &&
> +          (FunctionDeclarator || Pointer) && FTI.isAmbiguous &&
> +          S.CurContext->isFunctionOrMethod() &&
> +          D.getFunctionDefinitionKind() == FDK_Declaration &&
> +          D.getDeclSpec().getStorageClassSpecAsWritten()
> +          == DeclSpec::SCS_unspecified) {

We should also be checking (RT->isRecordType() || FTI.NumArgs <= 1) here.

On Wed, Jul 25, 2012 at 4:10 AM, Nikola Smiljanic <popizdeh at gmail.com>wrote:

> > Because that could never be valid as an initializer.
>
> I have no idea what I was thinking :)
>
> What I find confusing is that you're thinking in terms of when to
> suppress the warning and I'm looking for the opposite, conditions that
> need to be satisfied to emit the warning.
>
> If the return type is a reference then we only have to check that
> there is exactly one argument as this is the only valid way (that I
> know of) to initialize a reference. This suppresses the warning for
> zero arguments and more than one argument no matter if the return type
> is a class type or not.
>

I don't follow. If the return type is a reference, then it's not a class
type.


> Here's a new patch, the only failing test is this piece of
> objective-c++ code that I don't know what to do about? This is
> ambiguous if we disregard the attribute, but the attribute makes the
> fixit incorrect.
>
> [[noreturn]]int(e)();
>

Interesting. This would previously have warned if the parentheses around
'e' were dropped, so I don't really think this is a significant regression.
I think, even with that regression, the patch is still a massive
improvement; please add a FIXME to that test saying we shouldn't produce
the vexing-parse warning. Eventually we should walk the attributes on the
declarator, and if we find one for which a variable is not a valid subject
but a function is, then we should suppress the warning.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/afea5823/attachment.html>