[PATCH] PR11306 - Variadic template fix-it suggestion

[Jordan Rose]

By the way, it's considered good form to add cfe-commits as a CC on all Phabricator reviews. Not everyone uses Phabricator.

================
Comment at: lib/Parse/ParseTemplate.cpp:358-366
@@ -357,3 +357,11 @@
       // subsumed by whatever goes on in ParseTemplateParameter.
-      Diag(Tok.getLocation(), diag::err_expected_comma_greater);
+      if (Tok.is(tok::ellipsis)) {
+        FixItHint FixIt;
+        if (!TemplateParams.back()->isParameterPack())
+          FixIt = FixItHint::CreateInsertion(PrevTokLocation, "... ");
+        Diag(Tok.getLocation(), diag::err_misplaced_ellipsis_in_parameter_pack)
+          << FixItHint::CreateRemoval(Tok.getLocation()) << FixIt;
+      }
+      else
+        Diag(Tok.getLocation(), diag::err_expected_comma_greater);
       SkipUntil(tok::comma, tok::greater, tok::greatergreater,
----------------
Nikola Smiljanić wrote:
> Jordan Rose wrote:
> > This one looks fine, but since you're putting the fix-it on the error itself you have to then recover as if the user wrote the pack correctly.
> Thanks, I was thinking about this but I wasn't sure how recovery in this case is done? Do I have to modify the existing Decl? Could you point me to code that does something like this?
> 
> I have a test for this:
> 
>   template<typename Args...> // this would give misplaced ellipsis error
>   void baz(Args args...);  // this should give unexpanded parameter pack error but doesn't as Args is not a parameter pack
> 
> 
You could modify the existing decl by marking it as a pack (something that currently isn't allowed), but you might have better luck just creating a new decl and replacing the existing one.

I'm not quite sure what's the right way to go here. +Richard for comment.

================
Comment at: lib/Sema/SemaTemplateVariadic.cpp:253-254
@@ -252,1 +252,4 @@
+    DB << SourceRange(Location);
+  for (auto Location : Locations)
+    DB << FixItHint::CreateInsertion(PP.getLocForEndOfToken(Location), "...");
   return true;
----------------
Nikola Smiljanić wrote:
> Jordan Rose wrote:
> > This I'm not so sure about. The place to put the ... might not be directly after the parameter pack, and pretending it is seems questionable.
> Could you be a bit more specific, what do you mean by "might not be directly after"?. I'm looking at [temp.variadic] p5 but the only example there is:
> 
>   template<typename... Args>
>   void g(Args ... args)
>   {
>     f(args);
>   }
>   
Whatever's before the `...` gets duplicated. This, for example, is entirely legal:

  template<typename T>
  int f(T arg);
  
  template<typename... Args>
  std::vector<int> g(Args ... args)
  {
    return { f(args)... };
  }

  g(1, "2", 3.0);

http://reviews.llvm.org/D3604