[PATCH] D85157: [Sema] Add casting check for integer to fixed point conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

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