Improvements to vexing-parse warnings. Make the no-parameters case more

accurate by asking the parser whether there was an ambiguity rather than trying
to reverse-engineer it from the DeclSpec. Make the with-parameters case have
better diagnostics by using semantic information to drive the warning,
improving the diagnostics and adding a fixit.

Patch by Nikola Smiljanic. Some minor changes by me to suppress diagnostics for
declarations of the form 'T (*x)(...)', which seem to have a very high false
positive rate, and to reduce indentation in 'warnAboutAmbiguousFunction'.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@160998 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 125 insertions, 85 deletions

diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
index ea514d3826..328345413f 100644
--- a/lib/Parse/ParseDecl.cpp
+++ b/lib/Parse/ParseDecl.cpp
@@ -4318,17 +4318,14 @@ void Parser::ParseDirectDeclarator(Declarator &D) {
       // 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))
-          break;
-      }
+      bool IsAmbiguous = false;
+      if (getLangOpts().CPlusPlus && D.mayBeFollowedByCXXDirectInit() &&
+          !isCXXFunctionDeclarator(&IsAmbiguous))
+        break;
       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);
@@ -4445,7 +4442,7 @@ void Parser::ParseParenDeclarator(Declarator &D) {
   // function prototype scope, including parameter declarators.
   ParseScope PrototypeScope(this,
                             Scope::FunctionPrototypeScope|Scope::DeclScope);
-  ParseFunctionDeclarator(D, attrs, T, RequiresArg);
+  ParseFunctionDeclarator(D, attrs, T, false, RequiresArg);
   PrototypeScope.Exit();
 }
 
@@ -4471,6 +4468,7 @@ void Parser::ParseParenDeclarator(Declarator &D) {
 void Parser::ParseFunctionDeclarator(Declarator &D,
                                      ParsedAttributes &FirstArgAttrs,
                                      BalancedDelimiterTracker &Tracker,
+                                     bool IsAmbiguous,
                                      bool RequiresArg) {
   assert(getCurScope()->isFunctionPrototypeScope() &&
          "Should call from a Function scope");
@@ -4588,7 +4586,7 @@ void Parser::ParseFunctionDeclarator(Declarator &D,
   // Remember that we parsed a function type, and remember the attributes.
   D.AddTypeInfo(DeclaratorChunk::getFunction(HasProto,
                                              /*isVariadic=*/EllipsisLoc.isValid(),
-                                             EllipsisLoc,
+                                             IsAmbiguous, EllipsisLoc,
                                              ParamInfo.data(), ParamInfo.size(),
                                              DS.getTypeQualifiers(),
                                              RefQualifierIsLValueRef,
diff --git a/lib/Parse/ParseExpr.cpp b/lib/Parse/ParseExpr.cpp
index 7f268b523d..8d4668b954 100644
--- a/lib/Parse/ParseExpr.cpp
+++ b/lib/Parse/ParseExpr.cpp
@@ -2415,7 +2415,7 @@ ExprResult Parser::ParseBlockLiteralExpression() {
   } else {
     // Otherwise, pretend we saw (void).
     ParsedAttributes attrs(AttrFactory);
-    ParamInfo.AddTypeInfo(DeclaratorChunk::getFunction(true, false,
+    ParamInfo.AddTypeInfo(DeclaratorChunk::getFunction(true, false, false,
                                                        SourceLocation(),
                                                        0, 0, 0,
                                                        true, SourceLocation(),
diff --git a/lib/Parse/ParseExprCXX.cpp b/lib/Parse/ParseExprCXX.cpp
index 39ef38fdc0..b393d1ff67 100644
--- a/lib/Parse/ParseExprCXX.cpp
+++ b/lib/Parse/ParseExprCXX.cpp
@@ -804,7 +804,7 @@ ExprResult Parser::ParseLambdaExpressionAfterIntroducer(
 
     D.AddTypeInfo(DeclaratorChunk::getFunction(/*hasProto=*/true,
                                            /*isVariadic=*/EllipsisLoc.isValid(),
-                                           EllipsisLoc,
+                                           /*isAmbiguous=*/false, EllipsisLoc,
                                            ParamInfo.data(), ParamInfo.size(),
                                            DS.getTypeQualifiers(),
                                            /*RefQualifierIsLValueRef=*/true,
@@ -849,6 +849,7 @@ ExprResult Parser::ParseLambdaExpressionAfterIntroducer(
     ParsedAttributes Attr(AttrFactory);
     D.AddTypeInfo(DeclaratorChunk::getFunction(/*hasProto=*/true,
                      /*isVariadic=*/false,
+                     /*isAmbiguous=*/false,
                      /*EllipsisLoc=*/SourceLocation(),
                      /*Params=*/0, /*NumParams=*/0,
                      /*TypeQuals=*/0,
diff --git a/lib/Parse/ParseTentative.cpp b/lib/Parse/ParseTentative.cpp
index 8cf1c29078..1a4df4791b 100644
--- a/lib/Parse/ParseTentative.cpp
+++ b/lib/Parse/ParseTentative.cpp
@@ -671,7 +671,7 @@ Parser::TPResult Parser::TryParseDeclarator(bool mayBeAbstract,
       // initializer that follows the declarator. Note that ctor-style
       // initializers are not possible in contexts where abstract declarators
       // are allowed.
-      if (!mayBeAbstract && !isCXXFunctionDeclarator(false/*warnIfAmbiguous*/))
+      if (!mayBeAbstract && !isCXXFunctionDeclarator())
         break;
 
       // direct-declarator '(' parameter-declaration-clause ')'
@@ -1267,7 +1267,7 @@ Parser::TryParseDeclarationSpecifier(bool *HasMissingTypename) {
 /// '(' parameter-declaration-clause ')' cv-qualifier-seq[opt]
 ///         exception-specification[opt]
 ///
-bool Parser::isCXXFunctionDeclarator(bool warnIfAmbiguous) {
+bool Parser::isCXXFunctionDeclarator(bool *IsAmbiguous) {
 
   // C++ 8.2p1:
   // The ambiguity arising from the similarity between a function-style cast and
@@ -1304,23 +1304,13 @@ bool Parser::isCXXFunctionDeclarator(bool warnIfAmbiguous) {
     }
   }
 
-  SourceLocation TPLoc = Tok.getLocation();
   PA.Revert();
 
-  // In case of an error, let the declaration parsing code handle it.
-  if (TPR == TPResult::Error())
-    return true;
-
-  if (TPR == TPResult::Ambiguous()) {
-    // Function declarator has precedence over constructor-style initializer.
-    // Emit a warning just in case the author intended a variable definition.
-    if (warnIfAmbiguous)
-      Diag(Tok, diag::warn_parens_disambiguated_as_function_decl)
-        << SourceRange(Tok.getLocation(), TPLoc);
-    return true;
-  }
+  if (IsAmbiguous && TPR == TPResult::Ambiguous())
+    *IsAmbiguous = true;
 
-  return TPR == TPResult::True();
+  // In case of an error, let the declaration parsing code handle it.
+  return TPR != TPResult::False();
 }
 
 /// parameter-declaration-clause:
@@ -1344,7 +1334,7 @@ Parser::TPResult
 Parser::TryParseParameterDeclarationClause(bool *InvalidAsDeclaration) {
 
   if (Tok.is(tok::r_paren))
-    return TPResult::True();
+    return TPResult::Ambiguous();
 
   //   parameter-declaration-list[opt] '...'[opt]
   //   parameter-declaration-list ',' '...'
diff --git a/lib/Sema/DeclSpec.cpp b/lib/Sema/DeclSpec.cpp
index f3ec5656ea..d12ca78390 100644
--- a/lib/Sema/DeclSpec.cpp
+++ b/lib/Sema/DeclSpec.cpp
@@ -145,6 +145,7 @@ CXXScopeSpec::getWithLocInContext(ASTContext &Context) const {
 /// DeclaratorChunk::getFunction - Return a DeclaratorChunk for a function.
 /// "TheDeclarator" is the declarator that this will be added to.
 DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto, bool isVariadic,
+                                             bool isAmbiguous,
                                              SourceLocation EllipsisLoc,
                                              ParamInfo *ArgInfo,
                                              unsigned NumArgs,
@@ -173,6 +174,7 @@ DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto, bool isVariadic,
   I.Fun.AttrList                = 0;
   I.Fun.hasPrototype            = hasProto;
   I.Fun.isVariadic              = isVariadic;
+  I.Fun.isAmbiguous             = isAmbiguous;
   I.Fun.EllipsisLoc             = EllipsisLoc.getRawEncoding();
   I.Fun.DeleteArgInfo           = false;
   I.Fun.TypeQuals               = TypeQuals;
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 6dfd796f40..e511157366 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -5245,58 +5245,6 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
         FunctionTemplate->setInvalidDecl();
     }
 
-    // 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.
-    if (!FunctionTemplate && D.getFunctionDefinitionKind() == FDK_Declaration &&
-        CurContext->isFunctionOrMethod() &&
-        D.getNumTypeObjects() == 1 && D.isFunctionDeclarator() &&
-        D.getDeclSpec().getStorageClassSpecAsWritten()
-          == DeclSpec::SCS_unspecified) {
-      QualType T = R->getAs<FunctionType>()->getResultType();
-      DeclaratorChunk &C = D.getTypeObject(0);
-      if (!T->isVoidType() && C.Fun.NumArgs == 0 && !C.Fun.isVariadic &&
-          !C.Fun.hasTrailingReturnType() &&
-          C.Fun.getExceptionSpecType() == EST_None) {
-        SourceRange ParenRange(C.Loc, C.EndLoc);
-        Diag(C.Loc, diag::warn_empty_parens_are_function_decl) << ParenRange;
-
-        // If the declaration looks like:
-        //   T var1,
-        //   f();
-        // and name lookup finds a function named 'f', then the ',' was
-        // probably intended to be a ';'.
-        if (!D.isFirstDeclarator() && D.getIdentifier()) {
-          FullSourceLoc Comma(D.getCommaLoc(), SourceMgr);
-          FullSourceLoc Name(D.getIdentifierLoc(), SourceMgr);
-          if (Comma.getFileID() != Name.getFileID() ||
-              Comma.getSpellingLineNumber() != Name.getSpellingLineNumber()) {
-            LookupResult Result(*this, D.getIdentifier(), SourceLocation(),
-                                LookupOrdinaryName);
-            if (LookupName(Result, S))
-              Diag(D.getCommaLoc(), diag::note_empty_parens_function_call)
-                << FixItHint::CreateReplacement(D.getCommaLoc(), ";") << NewFD;
-          }
-        }
-        const CXXRecordDecl *RD = T->getAsCXXRecordDecl();
-        // Empty parens mean value-initialization, and no parens mean default
-        // initialization. These are equivalent if the default constructor is
-        // user-provided, or if zero-initialization is a no-op.
-        if (RD && RD->hasDefinition() &&
-            (RD->isEmpty() || RD->hasUserProvidedDefaultConstructor()))
-          Diag(C.Loc, diag::note_empty_parens_default_ctor)
-            << FixItHint::CreateRemoval(ParenRange);
-        else {
-          std::string Init = getFixItZeroInitializerForType(T);
-          if (Init.empty() && LangOpts.CPlusPlus0x)
-            Init = "{}";
-          if (!Init.empty())
-            Diag(C.Loc, diag::note_empty_parens_zero_initialize)
-              << FixItHint::CreateReplacement(ParenRange, Init);
-        }
-      }
-    }
-
     // C++ [dcl.fct.spec]p5:
     //   The virtual specifier shall only be used in declarations of
     //   nonstatic class member functions that appear within a
@@ -7915,10 +7863,10 @@ NamedDecl *Sema::ImplicitlyDefineFunction(SourceLocation Loc,
   (void)Error; // Silence warning.
   assert(!Error && "Error setting up implicit decl!");
   Declarator D(DS, Declarator::BlockContext);
-  D.AddTypeInfo(DeclaratorChunk::getFunction(false, false, SourceLocation(), 0,
-                                             0, 0, true, SourceLocation(),
+  D.AddTypeInfo(DeclaratorChunk::getFunction(false, false, false,
+                                             SourceLocation(), 0, 0, 0, true, 
+                                             SourceLocation(), SourceLocation(),
                                              SourceLocation(), SourceLocation(),
-                                             SourceLocation(),
                                              EST_None, SourceLocation(),
                                              0, 0, 0, 0, Loc, Loc, D),
                 DS.getAttributes(),
diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
index 84e8a7944d..fcea5a75a9 100644
--- a/lib/Sema/SemaType.cpp
+++ b/lib/Sema/SemaType.cpp
@@ -554,7 +554,8 @@ static void maybeSynthesizeBlockSignature(TypeProcessingState &state,
   // ...and *prepend* it to the declarator.
   declarator.AddInnermostTypeInfo(DeclaratorChunk::getFunction(
                              /*proto*/ true,
-                             /*variadic*/ false, SourceLocation(),
+                             /*variadic*/ false,
+                             /*ambiguous*/ false, SourceLocation(),
                              /*args*/ 0, 0,
                              /*type quals*/ 0,
                              /*ref-qualifier*/true, SourceLocation(),
@@ -2040,6 +2041,101 @@ static void checkQualifiedFunction(Sema &S, QualType T,
     << getFunctionQualifiersAsString(T->castAs<FunctionProtoType>());
 }
 
+/// Produce an approprioate diagnostic for an ambiguity between a function
+/// declarator and a C++ direct-initializer.
+static void warnAboutAmbiguousFunction(Sema &S, Declarator &D,
+                                       DeclaratorChunk &DeclType, QualType RT) {
+  const DeclaratorChunk::FunctionTypeInfo &FTI = DeclType.Fun;
+  assert(FTI.isAmbiguous && "no direct-initializer / function ambiguity");
+
+  // If the return type is void there is no ambiguity.
+  if (RT->isVoidType())
+    return;
+
+  // An initializer for a non-class type can have at most one argument.
+  if (!RT->isRecordType() && FTI.NumArgs > 1)
+    return;
+
+  // An initializer for a reference must have exactly one argument.
+  if (RT->isReferenceType() && FTI.NumArgs != 1)
+    return;
+
+  // Only warn if this declarator is declaring a function at block scope, and
+  // doesn't have a storage class (such as 'extern') specified.
+  if (!D.isFunctionDeclarator() ||
+      D.getFunctionDefinitionKind() != FDK_Declaration ||
+      !S.CurContext->isFunctionOrMethod() ||
+      D.getDeclSpec().getStorageClassSpecAsWritten()
+        != DeclSpec::SCS_unspecified)
+    return;
+
+  // Inside a condition, a direct initializer is not permitted. We allow one to
+  // be parsed in order to give better diagnostics in condition parsing.
+  if (D.getContext() == Declarator::ConditionContext)
+    return;
+
+  SourceRange ParenRange(DeclType.Loc, DeclType.EndLoc);
+
+  // Declaration with parameters, eg. "T var(T());".
+  if (FTI.NumArgs > 0 &&
+      D.getContext() != Declarator::ConditionContext) {
+    S.Diag(DeclType.Loc,
+           diag::warn_parens_disambiguated_as_function_declaration)
+        << ParenRange;
+
+    SourceRange Range = FTI.ArgInfo[0].Param->getSourceRange();
+    SourceLocation B = Range.getBegin();
+    SourceLocation E = S.PP.getLocForEndOfToken(Range.getEnd());
+    // FIXME: Maybe we should suggest adding braces instead of parens
+    // in C++11 for classes that don't have an initializer_list constructor.
+    S.Diag(B, diag::note_additional_parens_for_variable_declaration)
+      << FixItHint::CreateInsertion(B, "(")
+      << FixItHint::CreateInsertion(E, ")");
+  }
+
+  // Declaration without parameters, eg. "T var();".
+  if (FTI.NumArgs == 0) {
+    S.Diag(DeclType.Loc, diag::warn_empty_parens_are_function_decl)
+      << ParenRange;
+
+    // If the declaration looks like:
+    //   T var1,
+    //   f();
+    // and name lookup finds a function named 'f', then the ',' was
+    // probably intended to be a ';'.
+    if (!D.isFirstDeclarator() && D.getIdentifier()) {
+      FullSourceLoc Comma(D.getCommaLoc(), S.SourceMgr);
+      FullSourceLoc Name(D.getIdentifierLoc(), S.SourceMgr);
+      if (Comma.getFileID() != Name.getFileID() ||
+          Comma.getSpellingLineNumber() != Name.getSpellingLineNumber()) {
+        LookupResult Result(S, D.getIdentifier(), SourceLocation(),
+        Sema::LookupOrdinaryName);
+        if (S.LookupName(Result, S.getCurScope()))
+          S.Diag(D.getCommaLoc(), diag::note_empty_parens_function_call)
+            << FixItHint::CreateReplacement(D.getCommaLoc(), ";")
+            << D.getIdentifier();
+      }
+    }
+    const CXXRecordDecl *RD = RT->getAsCXXRecordDecl();
+    // Empty parens mean value-initialization, and no parens mean
+    // default initialization. These are equivalent if the default
+    // constructor is user-provided or if zero-initialization is a
+    // no-op.
+    if (RD && RD->hasDefinition() &&
+        (RD->isEmpty() || RD->hasUserProvidedDefaultConstructor()))
+      S.Diag(DeclType.Loc, diag::note_empty_parens_default_ctor)
+        << FixItHint::CreateRemoval(ParenRange);
+    else {
+      std::string Init = S.getFixItZeroInitializerForType(RT);
+      if (Init.empty() && S.LangOpts.CPlusPlus0x)
+        Init = "{}";
+      if (!Init.empty())
+        S.Diag(DeclType.Loc, diag::note_empty_parens_zero_initialize)
+          << FixItHint::CreateReplacement(ParenRange, Init);
+    }
+  }
+}
+
 static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
                                                 QualType declSpecType,
                                                 TypeSourceInfo *TInfo) {
@@ -2272,6 +2368,11 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
           << (D.getContext() == Declarator::AliasDeclContext ||
               D.getContext() == Declarator::AliasTemplateContext);
 
+      // If we see "T var();" or "T var(T());" at block scope, it is probably
+      // an attempt to initialize a variable, not a function declaration.
+      if (FTI.isAmbiguous)
+        warnAboutAmbiguousFunction(S, D, DeclType, T);
+
       if (!FTI.NumArgs && !FTI.isVariadic && !LangOpts.CPlusPlus) {
         // Simple void foo(), where the incoming T is the result type.
         T = Context.getFunctionNoProtoType(T);