r215408 - Reject varargs '...' in function prototype if there are more parameters after

Alexander Kornienko alexfh at google.com
Fri Aug 15 15:43:41 PDT 2014


This patch causes http://llvm.org/PR20660. Can you take a look?


On Tue, Aug 12, 2014 at 1:30 AM, Richard Smith <richard-llvm at metafoo.co.uk>
wrote:

> Author: rsmith
> Date: Mon Aug 11 18:30:23 2014
> New Revision: 215408
>
> URL: http://llvm.org/viewvc/llvm-project?rev=215408&view=rev
> Log:
> 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
> "...".
>
> Modified:
>     cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>     cfe/trunk/include/clang/Sema/Sema.h
>     cfe/trunk/lib/Parse/ParseDecl.cpp
>     cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp
>     cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp
>     cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp
>     cfe/trunk/test/Parser/cxx-variadic-func.cpp
>     cfe/trunk/test/Parser/cxx11-templates.cpp
>     cfe/trunk/test/SemaCXX/issue547.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Aug 11
> 18:30:23 2014
> @@ -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<DiagGroup<"ambiguous-ellipsis">>;
> +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<DiagGroup<"anonymous-pack-parens">>;
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Mon Aug 11 18:30:23 2014
> @@ -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.
>    ///
>
> Modified: cfe/trunk/lib/Parse/ParseDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Parse/ParseDecl.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseDecl.cpp Mon Aug 11 18:30:23 2014
> @@ -5426,6 +5426,15 @@ void Parser::ParseParameterDeclarationCl
>        // 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::ParseParameterDeclarationCl
>                                            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;
>      }
>
>
> Modified: cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp Mon Aug 11 18:30:23 2014
> @@ -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<sema::LambdaScopeInfo>(SI))
> +      return true;
> +  return false;
> +}
> +
>  /// \brief Diagnose all of the unexpanded parameter packs in the given
>  /// vector.
>  bool
>
> Modified: cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp (original)
> +++ cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp Mon Aug 11
> 18:30:23 2014
> @@ -1,5 +1,4 @@
>  // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
> -// expected-no-diagnostics
>
>  template<typename T> struct identity;
>  template<typename ...Types> struct tuple;
> @@ -22,7 +21,7 @@ template<typename T> struct is_same<T, T
>  template<typename T, typename ...Types>
>  struct X0 {
>    typedef identity<T(Types...)> function_pack_1;
> -  typedef identity<T(Types......)> variadic_function_pack_1;
> +  typedef identity<T(Types......)> variadic_function_pack_1; //
> expected-warning {{varargs}} expected-note {{pack}} expected-note {{insert
> ','}}
>    typedef identity<T(T...)> variadic_1;
>    typedef tuple<T(Types, ...)...> template_arg_expansion_1;
>  };
>
> Modified:
> cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp
> (original)
> +++ cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp Mon
> Aug 11 18:30:23 2014
> @@ -243,7 +243,7 @@ namespace FunctionTypes {
>    };
>
>    template<typename R, typename ...Types>
> -  struct Arity<R(Types......)> {
> +  struct Arity<R(Types......)> { // expected-warning {{varargs}}
> expected-note {{pack}} expected-note {{insert ','}}
>      static const unsigned value = sizeof...(Types);
>    };
>
>
> Modified: cfe/trunk/test/Parser/cxx-variadic-func.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx-variadic-func.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Parser/cxx-variadic-func.cpp (original)
> +++ cfe/trunk/test/Parser/cxx-variadic-func.cpp Mon Aug 11 18:30:23 2014
> @@ -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}}
>
> Modified: cfe/trunk/test/Parser/cxx11-templates.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx11-templates.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Parser/cxx11-templates.cpp (original)
> +++ cfe/trunk/test/Parser/cxx11-templates.cpp Mon Aug 11 18:30:23 2014
> @@ -7,3 +7,40 @@ struct S {
>
>  template <typename Ty = char>
>  static_assert(sizeof(Ty) != 1, "Not a char"); // expected-error {{a
> static_assert declaration cannot be a template}}
> +
> +namespace Ellipsis {
> +  template<typename ...T> void f(T t..., int n); // expected-error {{must
> immediately precede declared identifier}}
> +  template<typename ...T> void f(int n, T t...); // expected-error {{must
> immediately precede declared identifier}}
> +  template<typename ...T> void f(int n, T t, ...); // expected-error
> {{unexpanded parameter pack}}
> +  template<typename ...T> 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 at -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 at -1 {{insert ',' before '...' to silence
> this warning}}
> +             );
> +      void i(T (&), ...);
> +    }...);
> +  }
> +  template<typename ...T> 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 at -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<typename...T> void f(T n = 1 ...);
> +  // expected-warning at -1 {{creates a C-style varargs}}
> +  // expected-note at -2 {{place '...' immediately before declared
> identifier}}
> +  // expected-note at -3 {{insert ','}}
> +  // expected-error at -4 {{unexpanded parameter pack}}
> +}
>
> Modified: cfe/trunk/test/SemaCXX/issue547.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/issue547.cpp?rev=215408&r1=215407&r2=215408&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/issue547.cpp (original)
> +++ cfe/trunk/test/SemaCXX/issue547.cpp Mon Aug 11 18:30:23 2014
> @@ -27,32 +27,32 @@ struct classify_function<R(Args...) cons
>  };
>
>  template<typename R, typename ...Args>
> -struct classify_function<R(Args......)> {
> +struct classify_function<R(Args..., ...)> {
>    static const unsigned value = 5;
>  };
>
>  template<typename R, typename ...Args>
> -struct classify_function<R(Args......) const> {
> +struct classify_function<R(Args..., ...) const> {
>    static const unsigned value = 6;
>  };
>
>  template<typename R, typename ...Args>
> -struct classify_function<R(Args......) volatile> {
> +struct classify_function<R(Args..., ...) volatile> {
>    static const unsigned value = 7;
>  };
>
>  template<typename R, typename ...Args>
> -struct classify_function<R(Args......) const volatile> {
> +struct classify_function<R(Args..., ...) const volatile> {
>    static const unsigned value = 8;
>  };
>
>  template<typename R, typename ...Args>
> -struct classify_function<R(Args......) &&> {
> +struct classify_function<R(Args..., ...) &&> {
>    static const unsigned value = 9;
>  };
>
>  template<typename R, typename ...Args>
> -struct classify_function<R(Args......) const &> {
> +struct classify_function<R(Args..., ...) const &> {
>    static const unsigned value = 10;
>  };
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140816/4d93240f/attachment.html>


More information about the cfe-commits mailing list