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