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