[PATCH] PR11306 - Variadic template fix-it suggestion

[Richard Smith]

================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:716
@@ -715,1 +715,3 @@
+def err_misplaced_ellipsis_in_parameter_pack : Error<
+  "ellipsis comes before the name of template parameter pack">;
 def err_paren_sizeof_parameter_pack : Error<
----------------
Ismail Pazarbasi wrote:
> Nikola Smiljanić wrote:
> > Ismail Pazarbasi wrote:
> > > My 2 cents about this:
> > > * IMO, using '...' is clearer than "ellipsis". I think all occurrences of "ellipsis" diagnostics can be replaced with '...' to make messages clearer.
> > > * I think you can emphasize that `...` must follow `typename` or `class` in type parameter declaration to form a parameter pack, something like: "'...' must follow %select{typename|class} in template parameter pack" or "'...' must precede '<identifier>'" (the latter one is closer to `err_misplaced_ellipsis_in_declaration`)
> > I'll go with '...' instead of ellipsis as it does seem to be clearer and more consistent.
> > 
> > I am having some trouble with the exact spelling:
> > We don't seem to select between `typename` and `class` in any of the existing diagnostics and it seems to be hard to get this information. Now printing the exact identifier is achievable by casting to NamedDecl but is this really necessary? I'd say that it's not any more useful than if we said "must precede declared identifier". And this brings me to the idea of reusing `err_misplaced_ellipsis_in_declaration` in which case the message would say "'...' must immediately precede declared" or if we want to be more specific:
> > 
> > '...' must immediately precede declared parameter pack
> > '...' must immediately precede the name of parameter pack
> I missed non-type template parameter case. Taking this into account, `err_misplaced_ellipsis_in_declaration` makes more sense.
Please just reuse `err_misplaced_ellipsis_in_declaration`. "declared identifier" seems fine here; I don't think we need to be more specific.

================
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,
----------------
Jordan Rose wrote:
> 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.
I'd prefer for the misplaced ellipsis detection/recovery to be done earlier. If you do this in `ParseTypeParameter` / `ParseTemplateTemplateParameter`, you should be able to create the parameter with the right variadicness (and also do the right thing if the ellipsis is between the parameter and a default argument).

================
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;
----------------
Jordan Rose wrote:
> 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);
Right. I don't think we can assume we know where the ellipsis should go here. (Well, we could try all possible places and pick one that works, but that seems like massively overkill.)

http://reviews.llvm.org/D3604