samitolvanen 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< ---------------- aaron.ballman wrote: > 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 > 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. This produces 1500+ new warnings in the kernel, but yes, I believe these are warnings we expected to see already with `-Wcast-function-type`. Nathan's suggestion to disable the strict warning without `W=1` sounds reasonable to me. ================ 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 + ---------------- aaron.ballman wrote: > Do we need to use the triple here, or can we make this test target agnostic? I don't think it's needed. This was copied from the `-Wcast-function-type` test. ================ 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}} */ +} ---------------- aaron.ballman wrote: > 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 > ``` The first two seem to be OK, the last one does produce a warning here: ``` cast from 'const int (*)(int)' to 'f10 *' (aka 'int (*)(int)') converts to incompatible function type ``` 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
