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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
