[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Thanks for working on this! Needs a bit of work, but we should be able to get this in very soon methinks.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup<CompareFunctionPointers>, DefaultIgnore;
----------------
This diagnostic feels very bare bones, and doesn't explain why the user should care. It would be good to rephrase the message so that it can incorporate some kind of reason too.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup<CompareFunctionPointers>, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<
----------------
It's very rare that we set a warning to `DefaultIgnore`. What's the motivation for this?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:12715
+  if (!IsOrdered && LHSType->isFunctionPointerType() &&
+      RHSType->isFunctionPointerType() && !LHSIsNull && !RHSIsNull) {
+    Diag(Loc, diag::warn_typecheck_comparison_of_function_pointers)
----------------
Braces sadly need to go, as this is a one-line statement.


================
Comment at: clang/test/SemaCXX/compare-function-pointer.cpp:9
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // expected-warning {{comparison of function pointers}}
+bool ne0 = a != b;  // expected-warning {{comparison of function pointers}}
----------------
Please be sure to check that the names are output too.


================
Comment at: clang/test/SemaCXX/compare-function-pointer.cpp:17
 
-bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}}
-bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}}
+bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}} expected-warning {{comparison of function pointers}}
+bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}} expected-warning {{comparison of function pointers}}
----------------
It doesn't make sense to me that we would emit a warning when we're already emitting an error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141310/new/

https://reviews.llvm.org/D141310