[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;
----------------
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