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< ---------------- nathanchance wrote: > 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. Do we think this will be onerously chatty for C code bases in general? My intuition is that it will be reasonable -- folks who have enabled this warning want to know when they're type casting function pointers in odd ways. Looking at some test cases, I actually think this will behave more like users expect. e.g., https://godbolt.org/z/cWGecK1KG ================ Comment at: clang/test/Sema/warn-cast-function-type-strict.c:30 + g = (f7 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}} */ +} ---------------- Some other test cases I think we should try out: ``` typedef int (f8)(int *); typedef int (f9)(const int); typedef int (f10)(int); int foo(int array[static 12]); int bar(int i); const int baz(int i); f8 *h = (f8 *)foo; // Should be okay, types are the same after adjustment f9 *i = (f9 *)bar; // Should be okay, types are the same after adjustment f10 *j = (f10 *)baz; // Should be okay, types are the same after adjustment ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134831/new/ https://reviews.llvm.org/D134831 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
