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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked 5 inline comments as done.
vabridgers added a comment.

I updated the commit header with more details since the first submission was obviously too terse. @bjope, I believe the update should address your comments.



================
Comment at: clang/lib/Sema/SemaCast.cpp:2660
     return;
+  if (SrcType->isIntegerType() && DestType->isFixedPointType())
+    return;
----------------
bjope wrote:
> 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.
> }
> ```
> 
I improve the commit message detail. Please read and let me know if the intent is still not clear. 


================
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
----------------
bjope wrote:
> 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.
Well, I had a few choices, seems I picked the red pill compared to what you want :)   Look at the change, see if you're happy with that. Of course, the owner will need to agree.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85157/new/

https://reviews.llvm.org/D85157