bjope added inline comments.
================
Comment at: clang/lib/Sema/SemaCast.cpp:2660
return;
+ if (SrcType->isIntegerType() && DestType->isFixedPointType())
+ return;
----------------
Is this really the intention with the patch?
It does match the "summary" above, but isn't the warning well deserved for
int->fixed cast.
I also base that question on the you referred to this patch from our bugtracker
at Ericsson, which was about conversions from `long __fixed` to `long __fixed`.
So I kind of expected to find `(SrcType->isFixedPointType() &&
DestType->isFixedPointType())` here (similar to the checks above that avoids
warning when SrcType equals the DestType).
The test case I expected (related to our downstream bug report) would look like
this:
```
_Fract ff1(void);
void
foo(void)
{
(_Fract)ff1(); // No warning expected.
}
```
================
Comment at: clang/test/Sema/warn-bad-function-cast.c:2
// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wbad-function-cast
-triple x86_64-unknown-unknown -verify
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wbad-function-cast
-DFIXED_POINT -ffixed-point -triple x86_64-unknown-unknown -verify
// rdar://9103192
----------------
I doubt it is worth running the whole test twice just to test both with and
without `-ffixed-point`, specially since there are no differences in the
earlier tests when enabling `-ffixed-point`. So just adding `-ffixed-point` to
the pre-existing RUN-line would save us from running this test case a humongous
number of extra times during the next decade.
================
Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+ (void)(_Fract) if1(); // no warning
+#endif
----------------
This should be added before the line saying `/* All following casts issue
warning */`.
================
Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+ (void)(_Fract) if1(); // no warning
+#endif
----------------
bjope wrote:
> This should be added before the line saying `/* All following casts issue
> warning */`.
Is the `(void)` needed/relevant here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85157/new/
https://reviews.llvm.org/D85157
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
