<div dir="ltr"><div class="gmail_extra">I think Larisse is already investigating.<br><br><div class="gmail_quote">On Fri, Aug 15, 2014 at 3:43 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">This patch causes <a href="http://llvm.org/PR20660" target="_blank">http://llvm.org/PR20660</a>. Can you take a look?<div>
<div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 12, 2014 at 1:30 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard-llvm@metafoo.co.uk" target="_blank">richard-llvm@metafoo.co.uk</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rsmith<br>
Date: Mon Aug 11 18:30:23 2014<br>
New Revision: 215408<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215408&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215408&view=rev</a><br>
Log:<br>
Reject varargs '...' in function prototype if there are more parameters after<br>
it. Diagnose with recovery if it appears after a function parameter that was<br>
obviously supposed to be a parameter pack. Otherwise, warn if it immediately<br>
follows a function parameter pack, because the user most likely didn't intend<br>
to write a parameter pack followed by a C-style varargs ellipsis.<br>
<br>
This warning can be syntactically disabled by using ", ..." instead of "...".<br>
<br>
Modified:<br>
    cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td<br>
    cfe/trunk/include/clang/Sema/Sema.h<br>
    cfe/trunk/lib/Parse/ParseDecl.cpp<br>
    cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp<br>
    cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp<br>
    cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp<br>
    cfe/trunk/test/Parser/cxx-variadic-func.cpp<br>
    cfe/trunk/test/Parser/cxx11-templates.cpp<br>
    cfe/trunk/test/SemaCXX/issue547.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Aug 11 18:30:23 2014<br>
@@ -503,6 +503,17 @@ def note_bracket_depth : Note<<br>
 def err_misplaced_ellipsis_in_declaration : Error<<br>
   "'...' must %select{immediately precede declared identifier|"<br>
   "be innermost component of anonymous pack declaration}0">;<br>
+def warn_misplaced_ellipsis_vararg : Warning<<br>
+  "'...' in this location creates a C-style varargs function"<br>
+  "%select{, not a function parameter pack|}0">,<br>
+  InGroup<DiagGroup<"ambiguous-ellipsis">>;<br>
+def note_misplaced_ellipsis_vararg_existing_ellipsis : Note<<br>
+  "preceding '...' declares a function parameter pack">;<br>
+def note_misplaced_ellipsis_vararg_add_ellipsis : Note<<br>
+  "place '...' %select{immediately before declared identifier|here}0 "<br>
+  "to declare a function parameter pack">;<br>
+def note_misplaced_ellipsis_vararg_add_comma : Note<<br>
+  "insert ',' before '...' to silence this warning">;<br>
 def ext_abstract_pack_declarator_parens : ExtWarn<<br>
   "ISO C++11 requires a parenthesized pack declaration to have a name">,<br>
   InGroup<DiagGroup<"anonymous-pack-parens">>;<br>
<br>
Modified: cfe/trunk/include/clang/Sema/Sema.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/include/clang/Sema/Sema.h (original)<br>
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Aug 11 18:30:23 2014<br>
@@ -5585,6 +5585,10 @@ public:<br>
   // C++ Variadic Templates (C++0x [temp.variadic])<br>
   //===--------------------------------------------------------------------===//<br>
<br>
+  /// Determine whether an unexpanded parameter pack might be permitted in this<br>
+  /// location. Useful for error recovery.<br>
+  bool isUnexpandedParameterPackPermitted();<br>
+<br>
   /// \brief The context in which an unexpanded parameter pack is<br>
   /// being diagnosed.<br>
   ///<br>
<br>
Modified: cfe/trunk/lib/Parse/ParseDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/lib/Parse/ParseDecl.cpp (original)<br>
+++ cfe/trunk/lib/Parse/ParseDecl.cpp Mon Aug 11 18:30:23 2014<br>
@@ -5426,6 +5426,15 @@ void Parser::ParseParameterDeclarationCl<br>
       // Otherwise, we have something.  Add it and let semantic analysis try<br>
       // to grok it and add the result to the ParamInfo we are building.<br>
<br>
+      // Last chance to recover from a misplaced ellipsis in an attempted<br>
+      // parameter pack declaration.<br>
+      if (Tok.is(tok::ellipsis) &&<br>
+          (NextToken().isNot(tok::r_paren) ||<br>
+           (!ParmDeclarator.getEllipsisLoc().isValid() &&<br>
+            !Actions.isUnexpandedParameterPackPermitted())) &&<br>
+          Actions.containsUnexpandedParameterPacks(ParmDeclarator))<br>
+        DiagnoseMisplacedEllipsisInDeclarator(ConsumeToken(), ParmDeclarator);<br>
+<br>
       // Inform the actions module about the parameter declarator, so it gets<br>
       // added to the current scope.<br>
       Decl *Param = Actions.ActOnParamDeclarator(getCurScope(),<br>
@@ -5492,12 +5501,34 @@ void Parser::ParseParameterDeclarationCl<br>
                                           Param, DefArgToks));<br>
     }<br>
<br>
-    if (TryConsumeToken(tok::ellipsis, EllipsisLoc) &&<br>
-        !getLangOpts().CPlusPlus) {<br>
-      // We have ellipsis without a preceding ',', which is ill-formed<br>
-      // in C. Complain and provide the fix.<br>
-      Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis)<br>
+    if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {<br>
+      if (!getLangOpts().CPlusPlus) {<br>
+        // We have ellipsis without a preceding ',', which is ill-formed<br>
+        // in C. Complain and provide the fix.<br>
+        Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis)<br>
+            << FixItHint::CreateInsertion(EllipsisLoc, ", ");<br>
+      } else if (ParmDeclarator.getEllipsisLoc().isValid() ||<br>
+                 Actions.containsUnexpandedParameterPacks(ParmDeclarator)) {<br>
+        // It looks like this was supposed to be a parameter pack. Warn and<br>
+        // point out where the ellipsis should have gone.<br>
+        SourceLocation ParmEllipsis = ParmDeclarator.getEllipsisLoc();<br>
+        Diag(EllipsisLoc, diag::warn_misplaced_ellipsis_vararg)<br>
+          << ParmEllipsis.isValid() << ParmEllipsis;<br>
+        if (ParmEllipsis.isValid()) {<br>
+          Diag(ParmEllipsis,<br>
+               diag::note_misplaced_ellipsis_vararg_existing_ellipsis);<br>
+        } else {<br>
+          Diag(ParmDeclarator.getIdentifierLoc(),<br>
+               diag::note_misplaced_ellipsis_vararg_add_ellipsis)<br>
+            << FixItHint::CreateInsertion(ParmDeclarator.getIdentifierLoc(),<br>
+                                          "...")<br>
+            << !ParmDeclarator.hasName();<br>
+        }<br>
+        Diag(EllipsisLoc, diag::note_misplaced_ellipsis_vararg_add_comma)<br>
           << FixItHint::CreateInsertion(EllipsisLoc, ", ");<br>
+      }<br>
+<br>
+      // We can't have any more parameters after an ellipsis.<br>
       break;<br>
     }<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp Mon Aug 11 18:30:23 2014<br>
@@ -197,6 +197,20 @@ namespace {<br>
   };<br>
 }<br>
<br>
+/// \brief Determine whether it's possible for an unexpanded parameter pack to<br>
+/// be valid in this location. This only happens when we're in a declaration<br>
+/// that is nested within an expression that could be expanded, such as a<br>
+/// lambda-expression within a function call.<br>
+///<br>
+/// This is conservatively correct, but may claim that some unexpanded packs are<br>
+/// permitted when they are not.<br>
+bool Sema::isUnexpandedParameterPackPermitted() {<br>
+  for (auto *SI : FunctionScopes)<br>
+    if (isa<sema::LambdaScopeInfo>(SI))<br>
+      return true;<br>
+  return false;<br>
+}<br>
+<br>
 /// \brief Diagnose all of the unexpanded parameter packs in the given<br>
 /// vector.<br>
 bool<br>
<br>
Modified: cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp<br>
URL: <a href="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" target="_blank">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</a><br>


==============================================================================<br>
--- cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp (original)<br>
+++ cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p14.cpp Mon Aug 11 18:30:23 2014<br>
@@ -1,5 +1,4 @@<br>
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s<br>
-// expected-no-diagnostics<br>
<br>
 template<typename T> struct identity;<br>
 template<typename ...Types> struct tuple;<br>
@@ -22,7 +21,7 @@ template<typename T> struct is_same<T, T<br>
 template<typename T, typename ...Types><br>
 struct X0 {<br>
   typedef identity<T(Types...)> function_pack_1;<br>
-  typedef identity<T(Types......)> variadic_function_pack_1;<br>
+  typedef identity<T(Types......)> variadic_function_pack_1; // expected-warning {{varargs}} expected-note {{pack}} expected-note {{insert ','}}<br>
   typedef identity<T(T...)> variadic_1;<br>
   typedef tuple<T(Types, ...)...> template_arg_expansion_1;<br>
 };<br>
<br>
Modified: cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp<br>
URL: <a href="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" target="_blank">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</a><br>


==============================================================================<br>
--- cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp (original)<br>
+++ cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/metafunctions.cpp Mon Aug 11 18:30:23 2014<br>
@@ -243,7 +243,7 @@ namespace FunctionTypes {<br>
   };<br>
<br>
   template<typename R, typename ...Types><br>
-  struct Arity<R(Types......)> {<br>
+  struct Arity<R(Types......)> { // expected-warning {{varargs}} expected-note {{pack}} expected-note {{insert ','}}<br>
     static const unsigned value = sizeof...(Types);<br>
   };<br>
<br>
<br>
Modified: cfe/trunk/test/Parser/cxx-variadic-func.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx-variadic-func.cpp?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx-variadic-func.cpp?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/test/Parser/cxx-variadic-func.cpp (original)<br>
+++ cfe/trunk/test/Parser/cxx-variadic-func.cpp Mon Aug 11 18:30:23 2014<br>
@@ -1,5 +1,8 @@<br>
-// RUN: %clang_cc1 -fsyntax-only  %s<br>
+// RUN: %clang_cc1 -fsyntax-only -verify %s<br>
<br>
 void f(...) {<br>
-  int g(int(...));<br>
+  // FIXME: There's no disambiguation here; this is unambiguous.<br>
+  int g(int(...)); // expected-warning {{disambiguated}} expected-note {{paren}}<br>
 }<br>
+<br>
+void h(int n..., int m); // expected-error {{expected ')'}} expected-note {{to match}}<br>
<br>
Modified: cfe/trunk/test/Parser/cxx11-templates.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx11-templates.cpp?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx11-templates.cpp?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/test/Parser/cxx11-templates.cpp (original)<br>
+++ cfe/trunk/test/Parser/cxx11-templates.cpp Mon Aug 11 18:30:23 2014<br>
@@ -7,3 +7,40 @@ struct S {<br>
<br>
 template <typename Ty = char><br>
 static_assert(sizeof(Ty) != 1, "Not a char"); // expected-error {{a static_assert declaration cannot be a template}}<br>
+<br>
+namespace Ellipsis {<br>
+  template<typename ...T> void f(T t..., int n); // expected-error {{must immediately precede declared identifier}}<br>
+  template<typename ...T> void f(int n, T t...); // expected-error {{must immediately precede declared identifier}}<br>
+  template<typename ...T> void f(int n, T t, ...); // expected-error {{unexpanded parameter pack}}<br>
+  template<typename ...T> void f() {<br>
+    f([]{<br>
+      void g(T<br>
+             t // expected-note {{place '...' immediately before declared identifier to declare a function parameter pack}}<br>
+             ... // expected-warning {{'...' in this location creates a C-style varargs function, not a function parameter pack}}<br>
+             // expected-note@-1 {{insert ',' before '...' to silence this warning}}<br>
+             );<br>
+      void h(T (&<br>
+              ) // expected-note {{place '...' here to declare a function parameter pack}}<br>
+             ... // expected-warning {{'...' in this location creates a C-style varargs function, not a function parameter pack}}<br>
+             // expected-note@-1 {{insert ',' before '...' to silence this warning}}<br>
+             );<br>
+      void i(T (&), ...);<br>
+    }...);<br>
+  }<br>
+  template<typename ...T> struct S {<br>
+    void f(T t...); // expected-error {{must immediately precede declared identifier}}<br>
+    void f(T ... // expected-note {{preceding '...' declares a function parameter pack}}<br>
+           t...); // expected-warning-re {{'...' in this location creates a C-style varargs function{{$}}}}<br>
+           // expected-note@-1 {{insert ',' before '...' to silence this warning}}<br>
+  };<br>
+<br>
+  // FIXME: We should just issue a single error in this case pointing out where<br>
+  // the '...' goes. It's tricky to recover correctly in this case, though,<br>
+  // because the parameter is in scope in the default argument, so must be<br>
+  // passed to Sema before we reach the ellipsis.<br>
+  template<typename...T> void f(T n = 1 ...);<br>
+  // expected-warning@-1 {{creates a C-style varargs}}<br>
+  // expected-note@-2 {{place '...' immediately before declared identifier}}<br>
+  // expected-note@-3 {{insert ','}}<br>
+  // expected-error@-4 {{unexpanded parameter pack}}<br>
+}<br>
<br>
Modified: cfe/trunk/test/SemaCXX/issue547.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/issue547.cpp?rev=215408&r1=215407&r2=215408&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/issue547.cpp?rev=215408&r1=215407&r2=215408&view=diff</a><br>


==============================================================================<br>
--- cfe/trunk/test/SemaCXX/issue547.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/issue547.cpp Mon Aug 11 18:30:23 2014<br>
@@ -27,32 +27,32 @@ struct classify_function<R(Args...) cons<br>
 };<br>
<br>
 template<typename R, typename ...Args><br>
-struct classify_function<R(Args......)> {<br>
+struct classify_function<R(Args..., ...)> {<br>
   static const unsigned value = 5;<br>
 };<br>
<br>
 template<typename R, typename ...Args><br>
-struct classify_function<R(Args......) const> {<br>
+struct classify_function<R(Args..., ...) const> {<br>
   static const unsigned value = 6;<br>
 };<br>
<br>
 template<typename R, typename ...Args><br>
-struct classify_function<R(Args......) volatile> {<br>
+struct classify_function<R(Args..., ...) volatile> {<br>
   static const unsigned value = 7;<br>
 };<br>
<br>
 template<typename R, typename ...Args><br>
-struct classify_function<R(Args......) const volatile> {<br>
+struct classify_function<R(Args..., ...) const volatile> {<br>
   static const unsigned value = 8;<br>
 };<br>
<br>
 template<typename R, typename ...Args><br>
-struct classify_function<R(Args......) &&> {<br>
+struct classify_function<R(Args..., ...) &&> {<br>
   static const unsigned value = 9;<br>
 };<br>
<br>
 template<typename R, typename ...Args><br>
-struct classify_function<R(Args......) const &> {<br>
+struct classify_function<R(Args..., ...) const &> {<br>
   static const unsigned value = 10;<br>
 };<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><div><br></div>
</div></div></div></div>
</blockquote></div><br></div></div>