[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1597627 <https://reviews.llvm.org/D64666#1597627>, @jfb wrote:

> In D64666#1597193 <https://reviews.llvm.org/D64666#1597193>, @aaron.ballman wrote:
>
> > In D64666#1596660 <https://reviews.llvm.org/D64666#1596660>, @xbolva00 wrote:
> >
> > > I think we should warn in that case even if GCC does not warn.
> >
> >
> > Strong +1.
>
>
> Sorry if the phrasing was misleading: if we know for a fact that there's a problem, we should warn unconditionally. If we don't know for a fact then the warning should *not* be enabled by `-Wall` nor `-Wextra`. I don't really care what GCC does by default, LLVM doesn't have to match every single thing. That being said, if LLVM behaves differently then maybe the flag name should be different.
>
> > In D64666#1596853 <https://reviews.llvm.org/D64666#1596853>, @ziangwan wrote:
> > 
> >> Final review ping.
> > 
> > 
> > Please be sure to give reviewers enough time to respond to comments before pinging a review.
>
> Indeed. You haven't answered my first comment, I'd expect you to do so and not "final ping" anything. I'm not saying you must do what I say, just that you must answer comments, not ignore them.


Sorry to miss out your first comment, but I did answer it under @xbolva00's comment. I will make sure I put my response under the original one next time.

In D64666#1596655 <https://reviews.llvm.org/D64666#1596655>, @ziangwan wrote:

> In D64666#1596512 <https://reviews.llvm.org/D64666#1596512>, @xbolva00 wrote:
>
> > @jfb’s comment is not addressed yet.
> >
> > In D64666#1583629 <https://reviews.llvm.org/D64666#1583629>, @jfb wrote:
> >
> > > I think you want to default-ignore the "may lose precision" warnings, but not the ones that you know always lose precision.
> >
>
>
> In GCC, the int type -> float type conversion warning is disabled by default. When the user uses `-Wconversion` or `-Wimplicit-float-conversion`, both "definitely lose" warning and "may lose" warning are issued. The current patch works the same way as GCC.


I think we definitely should issue the warning that says "may lose" precision. The reason is that a "may lose" warning encourages developers to examine their code and definitely helps them capture potential precision loss bugs. Also, in most case, we won't be able to know the exact value of the integer type, e.g. variables. If the developers are certain they are going to do an int->float conversion, they can always write explicit conversion, and the "may lose" warnings will go away.


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

https://reviews.llvm.org/D64666