bjope added inline comments.
================
Comment at: clang/lib/Sema/SemaCast.cpp:2660
return;
+ if (SrcType->isIntegerType() && DestType->isFixedPointType())
+ return;
----------------
vabridgers wrote:
> 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.
I'm afraid you didn't update the summary in phabricator? (I've never managed to
do that using arcanist myself, after having updated the commit msg locally I
usually need to update the description in phabricator using the web ui).
================
Comment at: clang/lib/Sema/SemaCast.cpp:2660
return;
+ if (SrcType->isIntegerType() && DestType->isFixedPointType())
+ return;
----------------
bjope wrote:
> vabridgers wrote:
> > 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.
> I'm afraid you didn't update the summary in phabricator? (I've never managed
> to do that using arcanist myself, after having updated the commit msg locally
> I usually need to update the description in phabricator using the web ui).
> 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/lib/Sema/SemaCast.cpp:2660
return;
+ if (SrcType->isIntegerType() && DestType->isFixedPointType())
+ return;
----------------
bjope wrote:
> bjope wrote:
> > vabridgers wrote:
> > > 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.
> > I'm afraid you didn't update the summary in phabricator? (I've never
> > managed to do that using arcanist myself, after having updated the commit
> > msg locally I usually need to update the description in phabricator using
> > the web ui).
> > 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.
> > }
> > ```
> >
>
>
> 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:49
+#ifdef FIXED_POINT
+ (void)(_Fract) if1(); // no warning
+#endif
----------------
vabridgers wrote:
> bjope wrote:
> > bjope wrote:
> > > bjope wrote:
> > > > This should be added before the line saying `/* All following casts
> > > > issue warning */`.
> > > Is the `(void)` needed/relevant here?
> > As questioned earlier, shouldn't we expect a warning for this scenario?
> >
> > There is however a problem that we get the warning for _Fract to _Fract
> > conversion. And it would be nice with a more complete set of tests
> > involving both FixedPoint->FixedPoint, FixedPoint->Integer and
> > Integer->FixedPoint casts.
> If you have any *specific* suggestions for test cases, I'm open to that.
Add:
```
_Fract ff1(void);
```
And inside foo add these three tests (you'll need to add appropriate expects):
```
(_Fract)ff1(); // No warning expected.
(_Fract)if1(); // Warning expected.
(int)ff1(); // Warning expected.
```
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
