This is an archive of the discontinued LLVM Phabricator instance.

[REFACTOR] Sema::MatchTemplateParametersToScopeSpecifier
Needs ReviewPublic

Authored by faisalv on Apr 18 2014, 9:26 PM.

Details

Summary

Refactor MatchTemplateParameterToScopeSpecifier by:

  • breaking the behemoth into a bunch of smaller functions and associating them into an implementation class
  • separating some of the diagnostic details out into its own class.

I am submitting this refactoring for review (and not committing it right away) because i'm not convinced the refactoring
is a clear improvement - specifically, I wonder if this is making the code harder to read? Some seasoned thoughts on
this refactoring will be much appreciated.

This refactoring is being performed in anticipation of submitting patches for DR727 which will allow explicit specializations
to be defined in-class/out-of-class. Additionally there is a slight change in the diagnostic for PR19340 which Richard committed as: r206444.
The message now indicates that the specialization is merely missing an empty template header (and will become legal once that is provided post DR727).

Thanks!

Diff Detail

Event Timeline

faisalv updated this revision to Unknown Object (????).Apr 19 2014, 6:43 AM

Make sure the recently updated diagnostic diag::err_specialize_member_of_template is supplied all its arguments.

rsmith edited edge metadata.May 20 2014, 5:58 PM

Sorry for the delay, dug this out of my spam box.

I think some refactoring here seems like a good idea, but I wonder if we can avoid doubling the length of the code in the process.

lib/Sema/SemaTemplate.cpp
1584–1591

We don't use this pattern elsewhere; wrap the anonymous namespace around the entire class, please.

1588–1590

///

1606

diagnoseMissingEmptyList? I think it's clear from context that we're talking about a template parameter list.

1667–1674

Please don't explain history, just explain what this thing is now.

1678

Why the template/temploid distinction here?

1680–1691

Please remove; I suspect this comment is not useful to anyone but you, and probably won't be useful to even you when you come back to this code in a couple of years.

1695

I don't think a lambda in an assert adds clarity here; use #ifndef NDEBUG instead.

1731

Extra space after &.

1856

This comment doesn't look useful. (And nearby ones likewise.)

1866

Drop this; it's easy enough to do this from a debugger if this case is reached.

1876–1879

I don't think we need to explain this here; readers of this code are expected to understand the language rules well enough to know what this means.

1920

Why mark this as inline?