[PATCH] [REFACTOR] Sema::MatchTemplateParametersToScopeSpecifier

[Richard Smith]

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.

================
Comment at: lib/Sema/SemaTemplate.cpp:1584-1591
@@ +1583,10 @@
+
+namespace {
+  class MatchTemplateParametersToScopeSpecifierDiagnoser;
+}
+
+// This class is an implementation abstraction that separates out all 
+// the diagnostic details of Sema::MatchTemplateParametersToScopeSpecifier 
+// and collects it into a class.
+class ::MatchTemplateParametersToScopeSpecifierDiagnoser {
+  Sema &S;
----------------
We don't use this pattern elsewhere; wrap the anonymous namespace around the entire class, please.

================
Comment at: lib/Sema/SemaTemplate.cpp:1588-1590
@@ +1587,5 @@
+
+// This class is an implementation abstraction that separates out all 
+// the diagnostic details of Sema::MatchTemplateParametersToScopeSpecifier 
+// and collects it into a class.
+class ::MatchTemplateParametersToScopeSpecifierDiagnoser {
----------------
`///`

================
Comment at: lib/Sema/SemaTemplate.cpp:1606
@@ +1605,3 @@
+  // We don't have a template header, but we should.
+  void diagnoseMissingEmptyTPL(const QualType CurType) const {
+    diagnoseMissingEmptyTPL(
----------------
`diagnoseMissingEmptyList`? I think it's clear from context that we're talking about a template parameter list.

================
Comment at: lib/Sema/SemaTemplate.cpp:1667-1674
@@ +1666,10 @@
+
+// This class is an implementation abstraction that was born during 
+// a refactoring of Sema::MatchTemplateParametersToScopeSpecifier to accomplish 
+// the following:
+//   - disentangle some of the implementation details into constituent functions
+//     that do a better job of doing 'one' thing.
+//   - indicate that the various functions are associated
+// FIXME: If any of the functions below can be reused, please use your discretion
+// and promote them into Sema or Context as appropriate. 
+
----------------
Please don't explain history, just explain what this thing is now.

================
Comment at: lib/Sema/SemaTemplate.cpp:1678
@@ +1677,3 @@
+
+  static bool isExplicitlySpecializedTemploidOrFullySpecializedTemplate(
+      const CXXRecordDecl *Record) {
----------------
Why the template/temploid distinction here?

================
Comment at: lib/Sema/SemaTemplate.cpp:1680-1691
@@ +1679,14 @@
+      const CXXRecordDecl *Record) {
+    //
+    // template<class T> struct A {
+    //   template<class U> struct B { };
+    //   struct B2;
+    // };
+    // template<>
+    //   struct A<int> { template<class U> struct B3 { }; };  ==> A<int> true
+    // template<> template<class U>
+    //   struct A<char>::B<U*> { }; ==> A<char> & B<U*> false
+    // template<> struct A<int>::B3<char> { }; // B3<char> true
+    // template <> struct A<char>::B2 { }; // B2 true
+    //
+    if (Record->getTemplateSpecializationKind() == TSK_ExplicitSpecialization &&
----------------
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.

================
Comment at: lib/Sema/SemaTemplate.cpp:1695
@@ +1694,3 @@
+
+      assert(([](const CXXRecordDecl *Record) {
+        // We are either a fully specialized template class ...
----------------
I don't think a lambda in an assert adds clarity here; use `#ifndef NDEBUG` instead.

================
Comment at: lib/Sema/SemaTemplate.cpp:1731
@@ +1730,3 @@
+  splitScopeSpecifierIntoTypesForTPLMatching(
+      Sema & SemaRef, const CXXScopeSpec &ScopeSpecifier) {
+    // The sequence of nested types to which we will match up the template
----------------
Extra space after `&`.

================
Comment at: lib/Sema/SemaTemplate.cpp:1856
@@ +1855,3 @@
+      if (TemplateDecl *Template = TST->getTemplateName().getAsTemplateDecl()) {
+        // NeedNonemptyTemplateHeader = true;
+        return TemplateParameterListRequirement::NonEmpty;
----------------
This comment doesn't look useful. (And nearby ones likewise.)

================
Comment at: lib/Sema/SemaTemplate.cpp:1866
@@ +1865,3 @@
+    } else {
+      QTy.dump();
+      llvm_unreachable(
----------------
Drop this; it's easy enough to do this from a debugger if this case is reached.

================
Comment at: lib/Sema/SemaTemplate.cpp:1876-1879
@@ +1875,6 @@
+  //  - and, it was not explicitly specialized (e.g.
+  //      template<class> struct A { struct B { }; };
+  //      template<> struct A<int>::B { }; <-- explicitly specialized temploid 
+  //      A<char>::B <-- non-explicitly specialized temploid
+  // 
+  static bool isNonExplicitlySpecializedTemploidClass(
----------------
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.

================
Comment at: lib/Sema/SemaTemplate.cpp:1920
@@ +1919,3 @@
+  // Returns true if we matched/consumed the the CurTPL.
+  static inline bool matchEmptyTemplateParameterList(
+      const TemplateParameterList *const CurTPL, const QualType CurType,
----------------
Why mark this as `inline`?

http://reviews.llvm.org/D3433