[PATCH] D16579: Warn if friend function depends on template parameters.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1153
@@ +1152,3 @@
+def warn_non_template_friend : Warning<"friend declaration %q0 depends on "
+  "template parameter but is not a template function">,
+   InGroup<NonTemplateFriend>;
----------------
template function -> function template

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1155-1157
@@ +1154,5 @@
+   InGroup<NonTemplateFriend>;
+def note_non_template_friend : Note<"if this is intentional, add function "
+  "declaration to suppress the warning, if not - make sure the function "
+  "template has already been declared and use '<>'">;
+def note_befriend_template : Note<"to befriend a template specialization, "
----------------
Rather than this long conditional note, how about producing two notes here:

  note: declare function outside class template to suppress this warning
  note: use '<>' to befriend a template specialization

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1158-1159
@@ -1152,1 +1157,4 @@
+  "template has already been declared and use '<>'">;
+def note_befriend_template : Note<"to befriend a template specialization, "
+  "use '<>'">;
 
----------------
This note should also point out that you need to declare the function template prior to the class template.

================
Comment at: include/clang/Sema/Sema.h:9342
@@ +9341,3 @@
+  /// of proper templates, but they are needed for checks.
+  llvm::DenseSet<FunctionDecl *> FriendsOfTemplates;
+
----------------
This will produce diagnostics in a nondeterministic order, and you have no need of a set because there can never be any duplicates. Just use a `SmallVector`?

================
Comment at: include/clang/Sema/Sema.h:9342
@@ +9341,3 @@
+  /// of proper templates, but they are needed for checks.
+  llvm::DenseSet<FunctionDecl *> FriendsOfTemplates;
+
----------------
rsmith wrote:
> This will produce diagnostics in a nondeterministic order, and you have no need of a set because there can never be any duplicates. Just use a `SmallVector`?
You're missing serialization code for this for the PCH / preamble case.

================
Comment at: include/clang/Sema/Sema.h:9344-9345
@@ +9343,4 @@
+
+  /// \brief Check dependent friends functions for misinterpretation as template
+  /// ones.
+  void CheckDependentFriends();
----------------
friends -> friend
template ones -> function templates

================
Comment at: lib/Sema/SemaChecking.cpp:10468
@@ +10467,3 @@
+/// function types.
+class FunctionMatcher : public TypeVisitor<FunctionMatcher, bool> {
+  const Type *InstType;
----------------
This is a huge amount of complexity and maintenance burden for this check, and it still seems to some important cases wrong. Is there a simpler way we can get a largely-equivalent result? Or maybe we can just unconditionally produce both notes?

================
Comment at: lib/Sema/SemaChecking.cpp:10700
@@ +10699,2 @@
+  }
+}
----------------
`FriendsOfTemplates.clear()` here?

================
Comment at: lib/Sema/SemaDecl.cpp:8502-8507
@@ -8501,1 +8501,8 @@
 
+  if (isFriend && !NewFD->isInvalidDecl()) {
+    if (TemplateParamLists.empty() && !DC->isRecord() &&
+        !isFunctionTemplateSpecialization) {
+      FriendsOfTemplates.insert(NewFD);
+    }
+  }
+
----------------
This should also be conditioned on the function having a dependent type and this declaration not being a definition -- this will add all non-template friends to `FriendsOfTemplates`, whether they're friends of a class template or not.


http://reviews.llvm.org/D16579