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

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance 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<
----------------
kees wrote:
> aaron.ballman wrote:
> > 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?
> > 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?
> 
> I'd be for that. It'll be very noisy for the Linux kernel, but they are all instances we need to fix.
> 
> cc @nathanchance
> > 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?
> 
> I'd be for that. It'll be very noisy for the Linux kernel, but they are all instances we need to fix.
> 
> cc @nathanchance

Right, it would be quite noisy but we have already built an escape hatch for this type of scenario. We can just do what we have done for other warnings and disable it for "normal" builds to avoid disrupting the configurations with `-Werror` enabled (especially since we are not the only ones testing with tip of tree LLVM) then turn it on for `W=1` so that the build bots will catch new instances of the warnings while we clean up the other ones. I think such a diff would just look like:

```
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 6ae482158bc4..52bd7df84fd6 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -64,6 +64,7 @@ KBUILD_CFLAGS += -Wno-sign-compare
 KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
 KBUILD_CFLAGS += -Wno-tautological-constant-out-of-range-compare
 KBUILD_CFLAGS += $(call cc-disable-warning, unaligned-access)
+KBUILD_CFLAGS += $(call cc-disable-warning, cast-function-type-strict)
 endif

 endif
```

then that can just be reverted when we have all the instances cleaned up. It would be nice to get a game plan together for tackling all these because they appear to be quite numerous.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134831