[PATCH] D97831: [Clang][Sema] Implement GCC -Wcast-function-type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8368
+def warn_cast_function_type : Warning<
+  "cast between incompatible function types from %0 to %1">,
+  InGroup<CastFunctionType>, DefaultIgnore;
----------------
I'd find something like "cast from function type %0 to incompatible function type %1" to be easier to read. But I suppose %0 and %1 are the pointer types, not the function types, so perhaps "cast from %0 to %1 converts to incompatible function type" or something like that?


================
Comment at: clang/lib/Sema/SemaCast.cpp:1044-1048
+  // Allow integral type mismatch if their size are equal.
+  if (SrcType->isIntegralType(Context) && DestType->isIntegralType(Context))
+    if (Context.getTypeInfoInChars(SrcType).Width ==
+        Context.getTypeInfoInChars(DestType).Width)
+      return true;
----------------
In addition to allowing the cases where the sizes are the same width, should we also allow cases where the promoted types are the same width (eg, `int` versus `short`)? What does GCC do?


================
Comment at: clang/lib/Sema/SemaCast.cpp:1050-1055
+  llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+  StructuralEquivalenceContext Ctx(
+      Context, Context, NonEquivalentDecls, StructuralEquivalenceKind::Default,
+      false /*StrictTypeSpelling*/, false /*Complain*/,
+      false /*ErrorOnTagTypeMismatch*/);
+  return Ctx.IsEquivalent(SrcType, DestType);
----------------
I think a "same type" check (`Context.hasSameUnqualifiedType(SrcType, DestType)`) would be more appropriate here than a structural equivalence check.


================
Comment at: clang/lib/Sema/SemaCast.cpp:1067-1081
+  if (SrcType->isFunctionPointerType() && DestType->isFunctionPointerType()) {
+    SrcFTy = SrcType->castAs<PointerType>()
+                 ->getPointeeType()
+                 ->castAs<FunctionType>();
+    DstFTy = DestType->castAs<PointerType>()
+                 ->getPointeeType()
+                 ->castAs<FunctionType>();
----------------
This can be simplified a bit.


================
Comment at: clang/lib/Sema/SemaCast.cpp:1082-1083
+                 ->castAs<FunctionType>();
+  } else {
+    return true;
+  }
----------------
We should also handle the case where the source is of function type and the destination is a reference-to-function type.

Maybe also block pointer types?


================
Comment at: clang/lib/Sema/SemaCast.cpp:1090
+      return false;
+    if (auto PT = T->getAs<FunctionProtoType>())
+      return !PT->isVariadic() && PT->getNumParams() == 0;
----------------
Please use `auto *` here (or `const auto *` as the linter suggests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97831