[PATCH] D85157: [Sema] Add casting check for fixed to fixed point conversions
Vince Bridgers via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 7 03:08:37 PDT 2020
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
More information about the cfe-commits
mailing list