From 444d8c4517ce4f74b52d64cf168bf1b7850ec7f7 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 11 Aug 2014 23:30:23 +0000 Subject: [PATCH] Reject varargs '...' in function prototype if there are more parameters after it. Diagnose with recovery if it appears after a function parameter that was obviously supposed to be a parameter pack. Otherwise, warn if it immediately follows a function parameter pack, because the user most likely didn't intend to write a parameter pack followed by a C-style varargs ellipsis. This warning can be syntactically disabled by using ", ..." instead of "...". git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@215408 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticParseKinds.td | 11 +++++ include/clang/Sema/Sema.h | 4 ++ lib/Parse/ParseDecl.cpp | 41 ++++++++++++++++--- lib/Sema/SemaTemplateVariadic.cpp | 14 +++++++ test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp | 3 +- .../temp.variadic/metafunctions.cpp | 2 +- test/Parser/cxx-variadic-func.cpp | 7 +++- test/Parser/cxx11-templates.cpp | 37 +++++++++++++++++ test/SemaCXX/issue547.cpp | 12 +++--- 9 files changed, 115 insertions(+), 16 deletions(-) diff --git a/include/clang/Basic/DiagnosticParseKinds.td b/include/clang/Basic/DiagnosticParseKinds.td index 961c847084..8937449c26 100644 --- a/include/clang/Basic/DiagnosticParseKinds.td +++ b/include/clang/Basic/DiagnosticParseKinds.td @@ -503,6 +503,17 @@ def note_bracket_depth : Note< def err_misplaced_ellipsis_in_declaration : Error< "'...' must %select{immediately precede declared identifier|" "be innermost component of anonymous pack declaration}0">; +def warn_misplaced_ellipsis_vararg : Warning< + "'...' in this location creates a C-style varargs function" + "%select{, not a function parameter pack|}0">, + InGroup>; +def note_misplaced_ellipsis_vararg_existing_ellipsis : Note< + "preceding '...' declares a function parameter pack">; +def note_misplaced_ellipsis_vararg_add_ellipsis : Note< + "place '...' %select{immediately before declared identifier|here}0 " + "to declare a function parameter pack">; +def note_misplaced_ellipsis_vararg_add_comma : Note< + "insert ',' before '...' to silence this warning">; def ext_abstract_pack_declarator_parens : ExtWarn< "ISO C++11 requires a parenthesized pack declaration to have a name">, InGroup>; diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index f82974e274..b1707517b7 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -5585,6 +5585,10 @@ public: // C++ Variadic Templates (C++0x [temp.variadic]) //===--------------------------------------------------------------------===// + /// Determine whether an unexpanded parameter pack might be permitted in this + /// location. Useful for error recovery. + bool isUnexpandedParameterPackPermitted(); + /// \brief The context in which an unexpanded parameter pack is /// being diagnosed. /// diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index b4713659b4..76f1e08442 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -5426,6 +5426,15 @@ void Parser::ParseParameterDeclarationClause( // Otherwise, we have something. Add it and let semantic analysis try // to grok it and add the result to the ParamInfo we are building. + // Last chance to recover from a misplaced ellipsis in an attempted + // parameter pack declaration. + if (Tok.is(tok::ellipsis) && + (NextToken().isNot(tok::r_paren) || + (!ParmDeclarator.getEllipsisLoc().isValid() && + !Actions.isUnexpandedParameterPackPermitted())) && + Actions.containsUnexpandedParameterPacks(ParmDeclarator)) + DiagnoseMisplacedEllipsisInDeclarator(ConsumeToken(), ParmDeclarator); + // Inform the actions module about the parameter declarator, so it gets // added to the current scope. Decl *Param = Actions.ActOnParamDeclarator(getCurScope(), @@ -5492,12 +5501,34 @@ void Parser::ParseParameterDeclarationClause( Param, DefArgToks)); } - if (TryConsumeToken(tok::ellipsis, EllipsisLoc) && - !getLangOpts().CPlusPlus) { - // We have ellipsis without a preceding ',', which is ill-formed - // in C. Complain and provide the fix. - Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis) + if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) { + if (!getLangOpts().CPlusPlus) { + // We have ellipsis without a preceding ',', which is ill-formed + // in C. Complain and provide the fix. + Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis) + << FixItHint::CreateInsertion(EllipsisLoc, ", "); + } else if (ParmDeclarator.getEllipsisLoc().isValid() || + Actions.containsUnexpandedParameterPacks(ParmDeclarator)) { + // It looks like this was supposed to be a parameter pack. Warn and + // point out where the ellipsis should have gone. + SourceLocation ParmEllipsis = ParmDeclarator.getEllipsisLoc(); + Diag(EllipsisLoc, diag::warn_misplaced_ellipsis_vararg) + << ParmEllipsis.isValid() << ParmEllipsis; + if (ParmEllipsis.isValid()) { + Diag(ParmEllipsis, + diag::note_misplaced_ellipsis_vararg_existing_ellipsis); + } else { + Diag(ParmDeclarator.getIdentifierLoc(), + diag::note_misplaced_ellipsis_vararg_add_ellipsis) + << FixItHint::CreateInsertion(ParmDeclarator.getIdentifierLoc(), + "...") + << !ParmDeclarator.hasName(); + } + Diag(EllipsisLoc, diag::note_misplaced_ellipsis_vararg_add_comma) << FixItHint::CreateInsertion(EllipsisLoc, ", "); + } + + // We can't have any more parameters after an ellipsis. break; } diff --git a/lib/Sema/SemaTemplateVariadic.cpp b/lib/Sema/SemaTemplateVariadic.cpp index 8e4ce0d9da..6e317d573b 100644 --- a/lib/Sema/SemaTemplateVariadic.cpp +++ b/lib/Sema/SemaTemplateVariadic.cpp @@ -197,6 +197,20 @@ namespace { }; } +/// \brief Determine whether it's possible for an unexpanded parameter pack to +/// be valid in this location. This only happens when we're in a declaration +/// that is nested within an expression that could be expanded, such as a +/// lambda-expression within a function call. +/// +/// This is conservatively correct, but may claim that some unexpanded packs are +/// permitted when they are not. +bool Sema::isUnexpandedParameterPackPermitted() { + for (auto *SI : FunctionScopes) + if (isa(SI)) + return true; + return false; +} + /// \brief Diagnose all of the unexpanded parameter packs in the given /// vector. bool diff --git a/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp b/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp index bc249b5a08..ac9344dea1 100644 --- a/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp +++ b/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp @@ -1,5 +1,4 @@ // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s -// expected-no-diagnostics template struct identity; template struct tuple; @@ -22,7 +21,7 @@ template struct is_same { template struct X0 { typedef identity function_pack_1; - typedef identity variadic_function_pack_1; + typedef identity variadic_function_pack_1; // expected-warning {{varargs}} expected-note {{pack}} expected-note {{insert ','}} typedef identity variadic_1; typedef tuple template_arg_expansion_1; }; diff --git a/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp b/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp index 73cbd0749c..f1231f6111 100644 --- a/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp +++ b/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp @@ -243,7 +243,7 @@ namespace FunctionTypes { }; template - struct Arity { + struct Arity { // expected-warning {{varargs}} expected-note {{pack}} expected-note {{insert ','}} static const unsigned value = sizeof...(Types); }; diff --git a/test/Parser/cxx-variadic-func.cpp b/test/Parser/cxx-variadic-func.cpp index b9360d6ed1..98a34d3e1b 100644 --- a/test/Parser/cxx-variadic-func.cpp +++ b/test/Parser/cxx-variadic-func.cpp @@ -1,5 +1,8 @@ -// RUN: %clang_cc1 -fsyntax-only %s +// RUN: %clang_cc1 -fsyntax-only -verify %s void f(...) { - int g(int(...)); + // FIXME: There's no disambiguation here; this is unambiguous. + int g(int(...)); // expected-warning {{disambiguated}} expected-note {{paren}} } + +void h(int n..., int m); // expected-error {{expected ')'}} expected-note {{to match}} diff --git a/test/Parser/cxx11-templates.cpp b/test/Parser/cxx11-templates.cpp index 20aa9b9f31..ed0495281c 100644 --- a/test/Parser/cxx11-templates.cpp +++ b/test/Parser/cxx11-templates.cpp @@ -7,3 +7,40 @@ struct S { template static_assert(sizeof(Ty) != 1, "Not a char"); // expected-error {{a static_assert declaration cannot be a template}} + +namespace Ellipsis { + template void f(T t..., int n); // expected-error {{must immediately precede declared identifier}} + template void f(int n, T t...); // expected-error {{must immediately precede declared identifier}} + template void f(int n, T t, ...); // expected-error {{unexpanded parameter pack}} + template void f() { + f([]{ + void g(T + t // expected-note {{place '...' immediately before declared identifier to declare a function parameter pack}} + ... // expected-warning {{'...' in this location creates a C-style varargs function, not a function parameter pack}} + // expected-note@-1 {{insert ',' before '...' to silence this warning}} + ); + void h(T (& + ) // expected-note {{place '...' here to declare a function parameter pack}} + ... // expected-warning {{'...' in this location creates a C-style varargs function, not a function parameter pack}} + // expected-note@-1 {{insert ',' before '...' to silence this warning}} + ); + void i(T (&), ...); + }...); + } + template struct S { + void f(T t...); // expected-error {{must immediately precede declared identifier}} + void f(T ... // expected-note {{preceding '...' declares a function parameter pack}} + t...); // expected-warning-re {{'...' in this location creates a C-style varargs function{{$}}}} + // expected-note@-1 {{insert ',' before '...' to silence this warning}} + }; + + // FIXME: We should just issue a single error in this case pointing out where + // the '...' goes. It's tricky to recover correctly in this case, though, + // because the parameter is in scope in the default argument, so must be + // passed to Sema before we reach the ellipsis. + template void f(T n = 1 ...); + // expected-warning@-1 {{creates a C-style varargs}} + // expected-note@-2 {{place '...' immediately before declared identifier}} + // expected-note@-3 {{insert ','}} + // expected-error@-4 {{unexpanded parameter pack}} +} diff --git a/test/SemaCXX/issue547.cpp b/test/SemaCXX/issue547.cpp index bfec6e080b..2a9dd13adf 100644 --- a/test/SemaCXX/issue547.cpp +++ b/test/SemaCXX/issue547.cpp @@ -27,32 +27,32 @@ struct classify_function { }; template -struct classify_function { +struct classify_function { static const unsigned value = 5; }; template -struct classify_function { +struct classify_function { static const unsigned value = 6; }; template -struct classify_function { +struct classify_function { static const unsigned value = 7; }; template -struct classify_function { +struct classify_function { static const unsigned value = 8; }; template -struct classify_function { +struct classify_function { static const unsigned value = 9; }; template -struct classify_function { +struct classify_function { static const unsigned value = 10; }; -- 2.40.0