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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked an inline comment as done.
vabridgers added inline comments.


================
Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+  (void)(_Fract) if1(); // no warning
+#endif
----------------
bjope wrote:
> bjope wrote:
> > 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.
> > ```
> I still think it would be nice not to break the structure of this test. Tests seem to be divided into three categories:
> 
> /* Casts to void types are always OK.  */
> 
>   /* Casts to the same type or similar types are OK.  */
> 
> /* All following casts issue warning */
> 
> And you have currently inserted all new tests in the last section.
> 
When I've seen bew


================
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:
> > > 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.
> > > ```
> > I still think it would be nice not to break the structure of this test. Tests seem to be divided into three categories:
> > 
> > /* Casts to void types are always OK.  */
> > 
> >   /* Casts to the same type or similar types are OK.  */
> > 
> > /* All following casts issue warning */
> > 
> > And you have currently inserted all new tests in the last section.
> > 
> When I've seen bew
Done. ok to land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85157