[PATCH] D13854: Template class: emit better diagnostic in case of missing template argument list

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Sema/SemaCXXScopeSpec.cpp:775-787
@@ +774,15 @@
+    if (ClassTemplateDecl *CTD = Found.getAsSingle<ClassTemplateDecl>()) {
+      TemplateParameterList *TPL = CTD->getTemplateParameters();
+      assert(TPL && "NULL template parameter list");
+      std::string FixString = Identifier.getName();
+      FixString += "<";
+      bool FirstArg = true;
+      for (NamedDecl *TemplArg : *TPL) {
+        if (FirstArg)
+          FirstArg = false;
+        else
+          FixString += ", ";
+        FixString += TemplArg->getName();
+      }
+      FixString += ">";
+      Diag(IdentifierLoc, diag::err_missing_argument_list) << &Identifier
----------------
This isn't quite the right thing to do; it's looking at the template parameters of the class template rather than the current template parameters, and they could be quite different. Instead, you should do something like:

1) Check that the `Scope` (`S`) is a `TemplateParamScope`
2) Walk that scope, extract all of its declarations, and check that they match in type and kind the template parameters of `CTD`
3) If so, produce a suggestion using the names of those template parameters, as found in the scope

================
Comment at: lib/Sema/SemaCXXScopeSpec.cpp:789
@@ +788,3 @@
+      Diag(IdentifierLoc, diag::err_missing_argument_list) << &Identifier
+          << FixItHint::CreateReplacement(IdentifierLoc, FixString);
+      if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
----------------
A `FixItHint` should only be attached to an error or warning diagnostic if we recover as if the fix-it were applied. Maybe move this to a separate note diagnostic instead?

================
Comment at: lib/Sema/SemaCXXScopeSpec.cpp:790
@@ +789,3 @@
+          << FixItHint::CreateReplacement(IdentifierLoc, FixString);
+      if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
+        Diag(ND->getLocation(), diag::note_entity_declared_at) << &Identifier;
----------------
You don't need to do this; `ND` will always be the same as `CTD` here.

================
Comment at: lib/Sema/SemaCXXScopeSpec.cpp:791
@@ +790,3 @@
+      if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
+        Diag(ND->getLocation(), diag::note_entity_declared_at) << &Identifier;
+    }
----------------
Just pass `CTD` in here instead of using `&Identifier`.

================
Comment at: lib/Sema/SemaCXXScopeSpec.cpp:792-793
@@ -775,1 +791,4 @@
+        Diag(ND->getLocation(), diag::note_entity_declared_at) << &Identifier;
+    }
+    else if (TypeDecl *TD = Found.getAsSingle<TypeDecl>()) {
       Diag(IdentifierLoc, diag::err_expected_class_or_namespace)
----------------
No newline between `}` and `else`, please.

================
Comment at: lib/Sema/SemaCXXScopeSpec.cpp:796
@@ -776,2 +795,3 @@
           << QualType(TD->getTypeForDecl(), 0) << getLangOpts().CPlusPlus;
+    }
     else {
----------------
Likewise.


Repository:
  rL LLVM

http://reviews.llvm.org/D13854