[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8686
+def warn_cast_function_type_strict : Warning<warn_cast_function_type.Text>,
+  InGroup<CastFunctionTypeStrict>, DefaultIgnore;
 def err_cast_pointer_to_non_pointer_int : Error<
----------------
samitolvanen wrote:
> nickdesaulniers wrote:
> > I don't think we need a new group for a warning that only contains one diagnostic kind?
> > I don't think we need a new group for a warning that only contains one diagnostic kind?
> 
> I might have misunderstood something, but we seem to have plenty of groups with only one diagnostic kind. Is there a way to add a new warning flag without adding a diagnostic group? Most users of `-Wcast-function-type` wouldn't want to enable the stricter version, so I would prefer to keep this separate.
> 
> I did notice that some warnings don't define the group in DiagnosticGroups.td, but specify it directly here. For example, `InGroup<DiagGroup<"argument-outside-range">>`. I'm not sure if there are any benefits in doing so.
Typically we only define a group in DiagnosticGroups.td when the group is going to be used by more than one diagnostic, otherwise we prefer using `InGroup<DiagGroup<"whatever">>` to form one-off diagnostic groups.

However, in this case, I am wondering if we want to add `CastFunctionTypeStrict` to be a subgroup of `CastFunctionType` so that users who enable `-Wcast-function-type` get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?


================
Comment at: clang/lib/Sema/SemaCast.cpp:1095-1097
+  // For strict checks, ensure we have an exact match.
+  else if (DiagID == diag::warn_cast_function_type_strict)
+    return DiagID;
----------------
Coding style nit.


================
Comment at: clang/lib/Sema/SemaCast.cpp:1184
 
-    if (!checkCastFunctionType(Self, SrcExpr, DestType))
-      Self.Diag(OpRange.getBegin(), diag::warn_cast_function_type)
+    if (auto DiagID = checkCastFunctionType(Self, SrcExpr, DestType))
+      Self.Diag(OpRange.getBegin(), DiagID)
----------------
Same elsewhere (the type isn't spelled out in the initialization, so we prefer using an explicit type).


================
Comment at: clang/test/Sema/warn-cast-function-type-strict.c:1
+// RUN: %clang_cc1 -x c %s -fsyntax-only -Wcast-function-type-strict -triple x86_64-- -verify
+
----------------
Do we need to use the triple here, or can we make this test target agnostic?


================
Comment at: clang/test/SemaCXX/warn-cast-function-type-strict.cpp:1
+// RUN: %clang_cc1 -x c++ %s -fblocks -fsyntax-only -Wcast-function-type-strict -triple x86_64-- -verify
+
----------------
Same question about triples here.


================
Comment at: clang/test/SemaCXX/warn-cast-function-type-strict.cpp:32
+  a = (f1 *)x;
+  b = (f2 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function type}} */
+  b = reinterpret_cast<f2 *>(x); /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function type}} */
----------------
You should use `//` style comments here, this is a C++ test file anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134831