Page MenuHomePhabricator

PR11306 - Variadic template fix-it suggestion
ClosedPublic

Authored by nikola on May 4 2014, 4:16 AM.

Details

Summary

This patch adds new error for misplaced ellipsis in template parameter pack. Fixit is suggested for this error and unexpanded parameter pack error. Is new error here acceptable or should this be a note? This will also impact the wording.

Diff Detail

Event Timeline

nikola updated this revision to Diff 9062.May 4 2014, 4:16 AM
nikola retitled this revision from to PR11306 - Variadic template fix-it suggestion.
nikola updated this object.
nikola edited the test plan for this revision. (Show Details)
nikola added a reviewer: doug.gregor.
ismailp added inline comments.
include/clang/Basic/DiagnosticParseKinds.td
716 ↗(On Diff #9062)

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)
test/FixIt/fixit-parameter-pack.cpp
1 ↗(On Diff #9062)

Can we use an existing test instead of adding a new file? e.g. test/FixIt/fixit-cxx0x.cpp? That file already contains similar diagnostics and FixIts.

nikola added inline comments.May 20 2014, 9:32 PM
include/clang/Basic/DiagnosticParseKinds.td
716 ↗(On Diff #9062)

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

test/FixIt/fixit-parameter-pack.cpp
1 ↗(On Diff #9062)

I think that we can, I'll also add tests that use a struct to make sure we have the same result for template and function parameter packs.

jordan_rose added inline comments.
lib/Parse/ParseTemplate.cpp
358–366

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.

lib/Sema/SemaTemplateVariadic.cpp
253–254 ↗(On Diff #9062)

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.

nikola added inline comments.May 21 2014, 8:52 PM
lib/Parse/ParseTemplate.cpp
358–366

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
lib/Sema/SemaTemplateVariadic.cpp
253–254 ↗(On Diff #9062)

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);
}
jordan_rose added a subscriber: Unknown Object (MLST).

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

lib/Parse/ParseTemplate.cpp
358–366

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.

lib/Sema/SemaTemplateVariadic.cpp
253–254 ↗(On Diff #9062)

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);
rsmith added inline comments.May 27 2014, 5:02 PM
include/clang/Basic/DiagnosticParseKinds.td
716 ↗(On Diff #9062)

Please just reuse err_misplaced_ellipsis_in_declaration. "declared identifier" seems fine here; I don't think we need to be more specific.

lib/Parse/ParseTemplate.cpp
358–366

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).

lib/Sema/SemaTemplateVariadic.cpp
253–254 ↗(On Diff #9062)

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.)

nikola updated this revision to Diff 10117.Jun 4 2014, 5:28 PM

I'm not too happy with DiagnoseMisplacedEllipsis but it's still better than having it in two places. The rest should be good.

rsmith added inline comments.Jun 4 2014, 6:02 PM
lib/Parse/ParseTemplate.cpp
674

Please move this check to the callers in ParseDecl.cpp, and change this to an assert.

676–679

I don't really like the interface of this function:

  • It's very Declarator-specific, and yet is called in non-Declarator cases
  • It gives the wrong fixit in the template-type-parameter and template-template-parameter cases because it can't tell whether there was already an ellipsis.

Maybe split it into:

DiagnoseMisplacedEllipsis(SourceLocation NewLoc, SourceLocation CorrectLoc, bool AlreadyHasEllipsis);
DiagnoseMisplacedEllipsisInDeclarator(SourceLocation NewLoc, SourceLocation CorrectLoc, Declarator *D);

... with the second function calling the first?

test/FixIt/fixit-cxx0x.cpp
142

Typo Missplaced

nikola updated this revision to Diff 10119.EditedJun 4 2014, 10:33 PM

That's sooo much better. I'm still not checking if ellipsis exist on template parameter and template template paremeter, but I don't expect anyone to write something like

template <typename... T...>

Then again... :)

rsmith edited edge metadata.Jun 5 2014, 6:06 PM

Thanks, this looks much nicer.

lib/Parse/ParseTemplate.cpp
503

I don't think your AlreadyHasEllipsis argument here is correct; we might have parsed one a few lines above.

588

Likewise here.

nikola updated this revision to Diff 10163.Jun 5 2014, 6:46 PM
nikola edited edge metadata.

You might have missed the last part of my comment, I clicked submit too soon and had to edit it. In any case I think you're right, it's not hard to detect redundant ellipsis and provide more accurate recovery. Here it is, with tests for this scenario.

rsmith accepted this revision.Jun 5 2014, 7:09 PM
rsmith edited edge metadata.

LGTM, thanks.

(Yes, I did miss the last part of your comment and then forgot about the part I had seen...)

This revision is now accepted and ready to land.Jun 5 2014, 7:09 PM
nikola closed this revision.Jun 5 2014, 8:08 PM

Thanks for your valuable guidance. Committed in r210304.