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

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Looks good. Some possible improvements to make the diagnostic a bit more precise, but I'd be happy if you want to address some of those as follow-ups after this change.



================
Comment at: clang/lib/Sema/SemaCast.cpp:1041
+                                   ASTContext &Context) {
+  if (SrcType->isPointerType() && DestType->isPointerType())
+    return true;
----------------
I think we should also treat references like pointers here, and similarly for block pointers and ObjC pointers. Perhaps using `hasPointerRepresentation()` would make sense?


================
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;
----------------
ychen wrote:
> rsmith wrote:
> > 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?
> GCC does exactly that. I didn't find a way to do that by looking at TargetInfo. Maybe there is a way for clang?
Hm. The right thing to do would be to call `ABIInfo::isPromotableIntegerTypeForABI`, but that's defined in `CodeGen` so we can't call it from here without violating layering. I suppose we could pull it up into the target info in `Basic`, if you're prepared to do that kind of refactoring, but maybe for now we can skip this check and emit a few warnings that GCC doesn't.


================
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);
----------------
ychen wrote:
> rsmith wrote:
> > I think a "same type" check (`Context.hasSameUnqualifiedType(SrcType, DestType)`) would be more appropriate here than a structural equivalence check.
> Agreed.
Taking this a bit further: we could use `hasSimilarType` to allow changes in nested cv-qualifiers. But given that we've already allowed arbitrary differences in pointee types, I suppose this would only affect pointers-to-members, and it might be preferable to allow arbitrary changes between pointer-to-member types here (except in the MS ABI case where there are meaningful differences in pointer-to-member representations between classes).

What does GCC do with differences in pointer-to-member types in function parameter types?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97831