[Sema] Enable -Wimplicit-int-float-conversion for integral to floating point precision loss

[ziangwan]

Summary

These code:

long i = 222222222222L;
float a = 222222222222L:
float b = a + i;

Will now issue warnings:

line 2: implicit conversion from 'long' to 'float' changes value from 222222222222 to 222222221312 [-Wimplicit-float-conversion]
line 3: implicit conversion from 'long' to 'float' may lose precision. [-Wimplicit-float-conversion]

The same feature is present in GCC but not currently in clang.

clang-tidy currently doesn't have this warning as well.

[jfb]

I think you want to default-ignore the "may lose precision" warnings, but not the ones that you know always lose precision.

[srhines]

You have patch.patch in the diff above. You should drop that.

[ziangwan]

Removed patch.patch.

[xbolva00]

You can check also https://reviews.llvm.org/D52835. I hit there issue which I didn't know how to solve.

[nickdesaulniers]

Thanks for the patch @ziangwan !

clang/include/clang/Basic/DiagnosticSemaKinds.td

3261

may lose or just loses.

clang/lib/Sema/SemaChecking.cpp

11404

significand

11412

End the sentence w/ punctuation.

11417

should this use SourceBT rather than TargetBT?

11420

accuracy

:set spell if you're using vim

11421

Maybe negate this conditional and return early?

11437

Should this just be a generic Diag rather than a DiagRuntimeBehavior?

[etalvala]

clang/test/Sema/implicit-float-conversion.c

29 ↗

(On Diff #209591)

should this also test the case of:
`
long g = 222222222222L;
long h = g + a;
`
or is that unlikely to fail if the assignment-to-float variant succeeds?

[nickdesaulniers]

I also recommend re-titling the commit to something like:

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

[ziangwan]

In D64666#1583633, @xbolva00 wrote:

You can check also https://reviews.llvm.org/D52835. I hit there issue which I didn't know how to solve.

Hi David,

Can you elaborate what issues you encountered? Thank you.

[ziangwan]

Fix spelling / comment / naming issues.

[ziangwan]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3261

"may lose". Whether the precision loss really happens depends on the value of i (how many precision bits i has).

For example, float e = i where i is an integer. If i is 15 then there isn't precision loss. If i is 222222222, then there is precision loss.

clang/lib/Sema/SemaChecking.cpp

11417

Should be TargetBT. I've updated the naming convention to make it more clear.

[ziangwan]

clang/test/Sema/implicit-float-conversion.c

29 ↗

(On Diff #209591)

I feel like your case is covered in the testReturn() function.

[xbolva00]

In D64666#1583667, @ziangwan wrote:

In D64666#1583633, @xbolva00 wrote:

You can check also https://reviews.llvm.org/D52835. I hit there issue which I didn't know how to solve.

Hi David,

Can you elaborate what issues you encountered? Thank you.

I had duplicated warning for C++11+ - my new warning and C++11’s narrowing warning.

Did you run ‘ninja check-clang’?

[xbolva00]

clang/lib/Sema/SemaChecking.cpp

11430

Can you check my older patch + tests + discussion?

I had to use other way to get this string..

clang/test/Sema/implicit-float-conversion.c

13 ↗

(On Diff #209611)

Can you show the whole warning message?

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3261

This isn't done yet (reopening comment until may loses is respelled may lose).

clang/lib/Sema/SemaChecking.cpp

11416

You don't need TargetFloatValue until later? Maybe sink it's definition below closer to where it's used. That way we don't have to calculate this if the below condition is false.

11430

And I don't think there's a test for this case? Or at least one that checks the printed value?

[ziangwan]

In D64666#1583792, @xbolva00 wrote:

In D64666#1583667, @ziangwan wrote:

In D64666#1583633, @xbolva00 wrote:

You can check also https://reviews.llvm.org/D52835. I hit there issue which I didn't know how to solve.

Hi David,

Can you elaborate what issues you encountered? Thank you.

I had duplicated warning for C++11+ - my new warning and C++11’s narrowing warning.

Did you run ‘ninja check-clang’?

I ran ninja check-clang and got all passed.

[ziangwan]

Update diff:

1. fix the spelling issue may loses integer precision -> may lose precision
2. issue more accurate warnings when int->float conversion involves constant integer.

[ziangwan]

clang/lib/Sema/SemaChecking.cpp

11430

I have checked your older patch.

IIUC, you take the log of number of precision bits and print the floating point value out. E.g. 2555+E9. I am doing it differently. I print out all the precision bits. E.g. 2555677.000. The reason is that I think it is more clear to print out changes value from 222222222222 to 222222221312 than changes value from 222222222222 to 222222221+e3.

11430

Test cases are added

[nickdesaulniers]

In D64666#1583792, @xbolva00 wrote:

I had duplicated warning for C++11+ - my new warning and C++11’s narrowing warning.

Why would narrowing be C++11 specific? I would think that narrowing applies anywhere integrals are used?

[nickdesaulniers]

I verified the conversion values are correct in the precise warnings in the test cases. From that perspective, and the rest of the code LGTM. Please wait for review from another reviewer. Thanks for the patch.

clang/lib/Sema/SemaChecking.cpp

11426

This TODO doesn't add information about what to fix in the future. I think it's ok to remove, as its understood that additional levels of semantic analysis is costlier and costlier.

[xbolva00]

Probably @aaron.ballman would like to review this too..

[scanon]

clang/lib/Sema/SemaChecking.cpp

11429

Why don't we just check that the result of the first conversion is opOK? I don't think doing a round-trip check is required here.

[ziangwan]

Update diff.

1. Adopt scanon's comment. Check the opStatus of int literal->float conversion to determine precision loss. All tests passed.

[ziangwan]

clang/lib/Sema/SemaChecking.cpp

11429

I have changed the code to check the status of the first conversion only. Please review it again.

[scanon]

LGTM. Please get at least one additional reviewer's approval before merging, as this is a corner of clang that I don't work on often.

[ziangwan]

Ping. I am going to submit this patch if nobody objects.

[xbolva00]

@jfb’s comment is not addressed yet.

In 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.

[aaron.ballman]

In 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.

We don't often add off-by-default diagnostics because users rarely enable them, but given that this is for GCC compatibility and that matches the behavior of GCC, I think it's a reasonable idea.

LGTM aside from the small nits I commented on.

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

1 ↗

(On Diff #209928)

Please revert whatever has changed in this file -- it seems to be whitespace only (perhaps line endings?).

clang/lib/Sema/SemaChecking.cpp

11437

DiagRuntimeBehavior() seems reasonable to me -- we don't want to warn users about this in unevaluated contexts like a sizeof(), at least that I can think of.

clang/test/Sema/implicit-float-conversion.c

3 ↗

(On Diff #209928)

You can remove some of the spurious newlines from this file.

[xbolva00]

I think @jfb wanted to say that if we know we always lose the precision, we should warn even without -Wxyz, no?

[aaron.ballman]

In D64666#1596619, @xbolva00 wrote:

I think @jfb wanted to say that if we know we always lose the precision, we should warn even without -Wxyz, no?

Oh, good catch! I hadn't noticed that *both* diagnostics were DefaultIgnore. I can't think of a reason why we'd want the constant-checked version off by default because it seems like there's no way to have a false positive there.

[ziangwan]

In D64666#1596512, @xbolva00 wrote:

@jfb’s comment is not addressed yet.

In 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.

[xbolva00]

I think we should warn in that case even if GCC does not warn.

[ziangwan]

Update diff

1. Fix trailing whitespaces.

[ziangwan]

Final review ping.

[aaron.ballman]

In D64666#1596660, @xbolva00 wrote:

I think we should warn in that case even if GCC does not warn.

Strong +1.

In D64666#1596853, @ziangwan wrote:

Final review ping.

Please be sure to give reviewers enough time to respond to comments before pinging a review.

[jfb]

In D64666#1597193, @aaron.ballman wrote:

In 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, @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.

[ziangwan]

In D64666#1597627, @jfb wrote:

In D64666#1597193, @aaron.ballman wrote:

In 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, @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, @ziangwan wrote:

In D64666#1596512, @xbolva00 wrote:

@jfb’s comment is not addressed yet.

In 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.

[xbolva00]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3279

Drop ‘DefaultIgnore‘ here

[ziangwan]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3279

I wish both warnings to be off by default and can be turned on by -Wconversion (-Wimplicit-float-conversion).

[ziangwan]

Update diff.

1. Make "definitely lose" warning on by default.

[jfb]

Thanks, the update looks good.

One thing I just noticed: if I have a codebase with -Wimplicit-float-conversion it seems like updating clang will automatically turn the two new warnings on, correct? That seems good, but also difficult to roll out because I can't turns off just the two new warnings. In other words, if I want to adopt a new clang and I had -Wimplicit-float-conversion then the only options I have are: fix all the new warnings (which might take time given how much code I have), or turn off *all* of -Wimplicit-float-conversion. I think it makes sense to force fixing all warnings for the warning that's always a problem warn_impcast_integer_float_precision_constant, but for warn_impcast_integer_float_precision it would be nice if there were a -Wno-* which disables only warn_impcast_integer_float_precision and nothing else.
This would make it way easier to roll out a new clang.

[ziangwan]

In D64666#1598194, @jfb wrote:

Thanks, the update looks good.

One thing I just noticed: if I have a codebase with -Wimplicit-float-conversion it seems like updating clang will automatically turn the two new warnings on, correct? That seems good, but also difficult to roll out because I can't turns off just the two new warnings. In other words, if I want to adopt a new clang and I had -Wimplicit-float-conversion then the only options I have are: fix all the new warnings (which might take time given how much code I have), or turn off *all* of -Wimplicit-float-conversion. I think it makes sense to force fixing all warnings for the warning that's always a problem warn_impcast_integer_float_precision_constant, but for warn_impcast_integer_float_precision it would be nice if there were a -Wno-* which disables only warn_impcast_integer_float_precision and nothing else.
This would make it way easier to roll out a new clang.

Yes, you are right. I am thinking about making a new flag for the warning but make "definitely lose" one default on. How about -Wimplicit-int-float-conversion?

[jfb]

In D64666#1599524, @ziangwan wrote:

In D64666#1598194, @jfb wrote:

Thanks, the update looks good.

One thing I just noticed: if I have a codebase with -Wimplicit-float-conversion it seems like updating clang will automatically turn the two new warnings on, correct? That seems good, but also difficult to roll out because I can't turns off just the two new warnings. In other words, if I want to adopt a new clang and I had -Wimplicit-float-conversion then the only options I have are: fix all the new warnings (which might take time given how much code I have), or turn off *all* of -Wimplicit-float-conversion. I think it makes sense to force fixing all warnings for the warning that's always a problem warn_impcast_integer_float_precision_constant, but for warn_impcast_integer_float_precision it would be nice if there were a -Wno-* which disables only warn_impcast_integer_float_precision and nothing else.
This would make it way easier to roll out a new clang.

Yes, you are right. I am thinking about making a new flag for the warning but make "definitely lose" one default on. How about -Wimplicit-int-float-conversion?

SGTM. It can still be a sub-warning of -Wimplicit-float-conversion, I just want to make sure we can turn it off on its own.

[aaron.ballman]

In D64666#1599544, @jfb wrote:

In D64666#1599524, @ziangwan wrote:

In D64666#1598194, @jfb wrote:

Thanks, the update looks good.

One thing I just noticed: if I have a codebase with -Wimplicit-float-conversion it seems like updating clang will automatically turn the two new warnings on, correct? That seems good, but also difficult to roll out because I can't turns off just the two new warnings. In other words, if I want to adopt a new clang and I had -Wimplicit-float-conversion then the only options I have are: fix all the new warnings (which might take time given how much code I have), or turn off *all* of -Wimplicit-float-conversion. I think it makes sense to force fixing all warnings for the warning that's always a problem warn_impcast_integer_float_precision_constant, but for warn_impcast_integer_float_precision it would be nice if there were a -Wno-* which disables only warn_impcast_integer_float_precision and nothing else.
This would make it way easier to roll out a new clang.

Yes, you are right. I am thinking about making a new flag for the warning but make "definitely lose" one default on. How about -Wimplicit-int-float-conversion?

SGTM. It can still be a sub-warning of -Wimplicit-float-conversion, I just want to make sure we can turn it off on its own.

+1

[ziangwan]

Update diff.

1. Make the two warnings controlled by -Wimplicit-int-float-conversion, which is a subset of -Wimplicit-float-conversion.

[jfb]

Great, thanks!

[xbolva00]

Looks ok

clang/include/clang/Basic/DiagnosticGroups.td

739

Probably you dont need to add this since ImplicitFloatConversion is already there

[aaron.ballman]

LGTM, though please remove the changes to Conversion if they're not needed before committing.

[phosek]

I'm seeing some test failures which appear to have been introduced by this change:

******************** TEST 'Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -triple x86_64-apple-macosx10.6.7 -verify /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp Line 136: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp Line 138: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
2 errors generated.

--

********************
Testing: 0 .
FAIL: Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp (1096 of 15293)
******************** TEST 'Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -Wno-error=c++11-narrowing -triple x86_64-apple-macosx10.6.7 -verify /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -Wno-error=narrowing -triple x86_64-apple-macosx10.6.7 -verify /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp Line 124: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp Line 126: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
2 errors generated.

--

[ziangwan]

In D64666#1600354, @phosek wrote:

I'm seeing some test failures which appear to have been introduced by this change:

******************** TEST 'Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -triple x86_64-apple-macosx10.6.7 -verify /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp Line 136: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp Line 138: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
2 errors generated.

--

********************
Testing: 0 .
FAIL: Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp (1096 of 15293)
******************** TEST 'Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -Wno-error=c++11-narrowing -triple x86_64-apple-macosx10.6.7 -verify /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -Wno-error=narrowing -triple x86_64-apple-macosx10.6.7 -verify /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp Line 124: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
  File /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp Line 126: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792
2 errors generated.

--

This is the intended behavior of this warning. I will fix the test cases and add expected-warning to them. Sorry to miss these two failing cases.

[phosek]

Reverted in r366979.

[ziangwan]

Update diff.

1. Fix 2 failing test cases. I omitted these two. My mistake.
2. Fix trailing whitespaces.

Okay?

[MaskRay]

In D64666#1600445, @ziangwan wrote:

Update diff.

1. Fix 2 failing test cases. I omitted these two. My mistake.
2. Fix trailing whitespaces.

Okay?

// CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
template<typename T>
struct Agg {
  T t;
};
...
  Agg<float> f9 = {123456789};

error: constant expression evaluates to 123456789 which cannot be narrowed to type 'float'
note: insert an explicit cast to silence this issue
warning: implicit conversion from 'int' to 'float' changes value from 123456789 to 123456792

The third diagnostic warn_impcast_integer_float_precision_constant is redundant.

[xbolva00]

This is not okay. Your patch has also same C++11 and higher issues related to narrowing (multiple warnings) as my old patch.

I told you about my issue. I asked you if your patch has no such issues - you said no.

I has asked you if you ran ninja check-clang - you said everything passed.

Please revert you patch (edit: phosek did it)

[ziangwan]

My bad. I omit these two test cases. I apologize for my carelessness.

Yes, this warning definitely is a duplicate of c++11 narrowing, *when the code actually uses initialization-list syntax. For example, the following code would issue duplicated warnings for now:

float ff = {222222222222L};  
long a = 222222222222L;
float ffff = {a};

However, c++11 narrowing conversion does not handle these cases. It does not handle initialization without initialization-list syntax. It does not handle implicit conversion in expressions. It does not handle implicit conversion in argument passing as well.

void test(float b) {}

float f = 222222222222L; // no narrowing warning

long a = 222222222222L;
float fff = a; // no narrowing warning
float c = a + fff; // no narrowing warning of a from long to float

test(a); // no narrowing warning

Handling extra cases allow us to sanity-check code that does not use initialization-list syntax or has implicit conversion in expressions. I believe it is a valuable addition. A fix would be to let -Wimplicit-int-float-conversion skip initialization-list syntax. The other way would to augment c++ narrowing. What do you guys think is the proper way?

I will continue work on this patch. Please don't panic.

[ziangwan]

Update diff.

1. Silence the warning when C++11 narrowing is in effect on intialization-list expression.
2. Add test cases to test "no duplicated warnings for c++11 narrowing"
3. Updated test cases to handle 32-bit host machine.

[xbolva00]

Nice simple fix :) thanks

[xbolva00]

clang/lib/Sema/SemaChecking.cpp

11647

Typo

11648

C++11

[xbolva00]

(But please wait for @aaron.ballman)

[ziangwan]

Fix typos.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

11650

Do you want to perform the isa<> on OrigE or on E which has had paren and implicit casts stripped?

[ziangwan]

clang/lib/Sema/SemaChecking.cpp

11650

When we are dealing with Initialization list syntax expression, the out-most expression will be of type InitListExpr. Since the out-most expression is not ImplicitCastExpr, IgnoreParenImpCasts() has no effect on the OrigE object. In other words, OrigE and E will be the same when isa<InitListExpr>(OrigE) evaluates to true.

In this case, I personally prefer OrigE since it is the "non-processed" object. However, the rest of the function uses E only. I can change it to E.

[aaron.ballman]

LGTM!

clang/lib/Sema/SemaChecking.cpp

11650

Ah, I was thinking we had to contend with a case like ({1, 2, 3}) where there's a ParenExpr involved, but I am not certain that's a case we have to worry about.

[ziangwan]

clang/lib/Sema/SemaChecking.cpp

11650

I think ({1,2,3}) is not a valid initializer.

implicit-int-float-narrowing.cpp:6:16: warning: expression result unused [-Wunused-value]
  int a[3] = ({1,2,3});
               ^
implicit-int-float-narrowing.cpp:6:18: warning: expression result unused [-Wunused-value]
  int a[3] = ({1,2,3});
                 ^
implicit-int-float-narrowing.cpp:6:21: error: expected ';' after expression
  int a[3] = ({1,2,3});
                    ^
implicit-int-float-narrowing.cpp:6:7: error: array initializer must be an initializer list
  int a[3] = ({1,2,3});
      ^

If so, I don think we need to worry about there's a ParenExpr involved.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

11650

Agreed. :-)

[ziangwan]

I have committed this patch. I will stay vigilant in case anything fails.

[ziangwan]

Buildbot failed for x86_64 target. Fix in progress.

[ziangwan]

Fixed.

[mgorny]

This change seems to have broken libc++ tests when run with trunk clang:
http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21221/steps/run%20unit%20tests/logs/FAIL%3A%20libc%2B%2B%3A%3Aeval.pass.cpp
and more in http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21221

[ziangwan]

The two failing cases I am seeing are:

/home/motus/netbsd8/netbsd8/llvm/projects/libcxx/include/random:3648:40: error: implicit conversion from 'unsigned int' to 'float' changes value from 2147483645 to 2147483648 [-Werror,-Wimplicit-int-float-conversion]
    const _RealType _Rp = _URNG::max() - _URNG::min() + _RealType(1);
                          ~~~~~~~~~~~~~^~~~~~~~~~~~~~ ~

/data/motus/netbsd8/netbsd8/llvm/projects/libcxx/test/std/numerics/rand/rand.util/rand.util.canonical/generate_canonical.pass.cpp:27:64: error: implicit conversion from 'unsigned int' to 'float' changes value from 2147483645 to 2147483648 [-Werror,-Wimplicit-int-float-conversion]
        assert(f == truncate_fp((16807 - E::min()) / (E::max() - E::min() + F(1))));
                                                      ~~~~~~~~~^~~~~~~~~~ ~

These are the intended behavior of this warning.

I will make a separate patch to fix the two failing cases.

[ziangwan]

In D64666#1612947, @mgorny wrote:

This change seems to have broken libc++ tests when run with trunk clang:
http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21221/steps/run%20unit%20tests/logs/FAIL%3A%20libc%2B%2B%3A%3Aeval.pass.cpp
and more in http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21221

The warning is actually correct here. This implicit integral to float conversion loses precision. Is it the intended behavior of the code? If so, we can simply add an explicit type cast to silence the warning.

[mgorny]

In D64666#1613057, @ziangwan wrote:

The warning is actually correct here. This implicit integral to float conversion loses precision. Is it the intended behavior of the code? If so, we can simply add an explicit type cast to silence the warning.

I don't really know. Hence, I added author of the code and libc++ maintainers as subscribers. Do you think I should open a bug for it too?

[ziangwan]

In D64666#1613356, @mgorny wrote:

In D64666#1613057, @ziangwan wrote:

The warning is actually correct here. This implicit integral to float conversion loses precision. Is it the intended behavior of the code? If so, we can simply add an explicit type cast to silence the warning.

I don't really know. Hence, I added author of the code and libc++ maintainers as subscribers. Do you think I should open a bug for it too?

We can wait for libc++ maintainers to respond before we open a bug.

Here is more insight of this warning:

float_type a = unsigned_int_type::max() - unsigned_int_type::min() + float_type(1);

The max() - min() expression evaluates to 2147483645 as type unsigned int. It then gets converted to float type, loses precision and becomes 2147483648f, where the significand is all 1. Then, 1f is added to 2147483648f. The final value is 2147483648f because 1 is too small compared to 2147483648f and has no effect. I don't know if the above is the intended behavior. Eventually, the final value is a floating point type with significand all ones. I guess that's what the author wants after all.

However, when it becomes (the expression is templated):

double_type a = unsigned_long_type::max() - unsigned_long_type::min() + double_type(1);

The behavior is different. max() - min() evaluates to 18446744073709551615. When it gets converted to double, the value changes to 18446744073709551616, where the significand is like 10...0 (a single one followed by many zeros). 1 is added to it and has no effect as well.

[sylvestre.ledru]

FYI, it found a bunch of new warnings in Firefox:
https://bugzilla.mozilla.org/show_bug.cgi?id=1577236
https://bugzilla.mozilla.org/show_bug.cgi?id=1577051

[sylvestre.ledru]

@ziangwan maybe you should add this improvement to the release notes, wdyt?

[srhines]

In D64666#1650325, @sylvestre.ledru wrote:

@ziangwan maybe you should add this improvement to the release notes, wdyt?

@sylvestre.ledru It's really great to hear that this new diagnostic is helpful. Thanks for the great suggestion.

@kongyi Can you take care of adding this to the release notes since @ziangwan has returned to university? I think that this really does deserve to be mentioned there. Thanks.

clang/include/clang/Basic/DiagnosticGroups.td

 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;
 def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">;
 def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,
                                                    UndefinedBoolConversion]>;
 def IntConversion : DiagGroup<"int-conversion">;
 def EnumConversion : DiagGroup<"enum-conversion">;
 def ImplicitIntConversion : DiagGroup<"implicit-int-conversion">;
-def ImplicitFloatConversion : DiagGroup<"implicit-float-conversion">;
+def ImplicitIntFloatConversion : DiagGroup<"implicit-int-float-conversion">;
+def ImplicitFloatConversion : DiagGroup<"implicit-float-conversion", [ImplicitIntFloatConversion]>;
 def ImplicitFixedPointConversion : DiagGroup<"implicit-fixed-point-conversion">;
 
 def FloatOverflowConversion : DiagGroup<"float-overflow-conversion">;
 def FloatZeroConversion : DiagGroup<"float-zero-conversion">;
 def FloatConversion :
   DiagGroup<"float-conversion", [FloatOverflowConversion,
                                  FloatZeroConversion]>;
 
def Conversion : DiagGroup<"conversion",
                             ConstantConversion,
                             EnumConversion,
                             BitFieldEnumConversion,
                             FloatConversion,
                             Shorten64To32,
                             IntConversion,
                             ImplicitIntConversion,
                             ImplicitFloatConversion,
                             LiteralConversion,
                             NonLiteralNullConversion, // (1-1)->pointer (etc)
                             NullConversion, // NULL->non-pointer
                             ObjCLiteralConversion,
                             SignConversion,
                             StringConversion]>,
                  DiagCategory<"Value Conversion Issue">;
 
 def Unused : DiagGroup<"unused",

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_impcast_bitfield_precision_constant : Warning<
   "implicit truncation from %2 to bit-field changes value from %0 to %1">,
   InGroup<BitFieldConstantConversion>;
 def warn_impcast_constant_int_to_objc_bool : Warning<
   "implicit conversion from constant value %0 to BOOL; "
   "the only well defined values for BOOL are YES and NO">,
   InGroup<ObjCBoolConstantConversion>;
 
 def warn_impcast_fixed_point_range : Warning<
   "implicit conversion from %0 cannot fit within the range of values for %1">,
   InGroup<ImplicitFixedPointConversion>;
 
 def warn_impcast_literal_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup<LiteralConversion>;
 def warn_impcast_literal_float_to_integer_out_of_range : Warning<
   "implicit conversion of out of range value from %0 to %1 is undefined">,
   InGroup<LiteralConversion>;
 def warn_impcast_float_integer : Warning<
   "implicit conversion turns floating-point number into integer: %0 to %1">,
   InGroup<FloatConversion>, DefaultIgnore;
 
+// Implicit int -> float conversion precision loss warnings.
+def warn_impcast_integer_float_precision : Warning<
+  "implicit conversion from %0 to %1 may lose precision">,
+  InGroup<ImplicitIntFloatConversion>, DefaultIgnore;
+def warn_impcast_integer_float_precision_constant : Warning<
+  "implicit conversion from %2 to %3 changes value from %0 to %1">,
+  InGroup<ImplicitIntFloatConversion>;
+
 def warn_impcast_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup<FloatOverflowConversion>, DefaultIgnore;
 def warn_impcast_float_to_integer_out_of_range : Warning<
   "implicit conversion of out of range value from %0 to %1 is undefined">,
   InGroup<FloatOverflowConversion>, DefaultIgnore;
 def warn_impcast_float_to_integer_zero : Warning<
   "implicit conversion from %0 to %1 changes non-zero value from %2 to %3">,

clang/lib/Sema/SemaChecking.cpp

if (Source->isIntegerType()) {
                                     << E->getSourceRange()
                                     << clang::SourceRange(CC));
           return;
         }
       }
     }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the integer type. Issue warnings for that accuracy loss.
+  if (SourceBT && TargetBT && SourceBT->isIntegerType() &&
+      TargetBT->isFloatingType()) {
+    // Determine the number of precision bits in the source integer type.
+    IntRange SourceRange = GetExprRange(S.Context, E, S.isConstantEvaluated());
+    unsigned int SourcePrecision = SourceRange.Width;
+
+    // Determine the number of precision bits in the
+    // target floating point type.
+    unsigned int TargetPrecision = llvm::APFloatBase::semanticsPrecision(
+        S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)));
+
+    if (SourcePrecision > 0 && TargetPrecision > 0 &&
+        SourcePrecision > TargetPrecision) {
+
+      llvm::APSInt SourceInt;
+      if (E->isIntegerConstantExpr(SourceInt, S.Context)) {
+        // If the source integer is a constant, convert it to the target
+        // floating point type. Issue a warning if the value changes
+        // during the whole conversion.
+        llvm::APFloat TargetFloatValue(
+            S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)));
+        llvm::APFloat::opStatus ConversionStatus =
+            TargetFloatValue.convertFromAPInt(
+                SourceInt, SourceBT->isSignedInteger(),
+                llvm::APFloat::rmNearestTiesToEven);
+
+        if (ConversionStatus != llvm::APFloat::opOK) {
+          std::string PrettySourceValue = SourceInt.toString(10);
+          SmallString<32> PrettyTargetValue;
+          TargetFloatValue.toString(PrettyTargetValue, TargetPrecision);
+
+          S.DiagRuntimeBehavior(
+              E->getExprLoc(), E,
+              S.PDiag(diag::warn_impcast_integer_float_precision_constant)
+                  << PrettySourceValue << PrettyTargetValue << E->getType() << T
+                  << E->getSourceRange() << clang::SourceRange(CC));
+        }
+      } else {
+        // Otherwise, the implicit conversion may lose precision.
+        DiagnoseImpCast(S, E, T, CC,
+                        diag::warn_impcast_integer_float_precision);
+      }
+    }
+  }
+
   DiagnoseNullConversion(S, E, T, CC);
 
   S.DiscardMisalignedMemberAddress(Target, E);
 
   if (!Source->isIntegerType() || !Target->isIntegerType())
     return;
 
   // TODO: remove this early return once the false positives for constant->bool

 /// implicit conversions in the given expression.  There are a couple
 /// of competing diagnostics here, -Wconversion and -Wsign-compare.
 static void AnalyzeImplicitConversions(Sema &S, Expr *OrigE,
                                        SourceLocation CC) {
   QualType T = OrigE->getType();
   Expr *E = OrigE->IgnoreParenImpCasts();
 
   if (E->isTypeDependent() || E->isValueDependent())
     return;
 
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
   if (isa<ConditionalOperator>(E)) {
     ConditionalOperator *CO = cast<ConditionalOperator>(E);
     CheckConditionalOperator(S, CO, CC, T);
     return;
   }
 
   // Check implicit argument conversions for function calls.
   if (CallExpr *Call = dyn_cast<CallExpr>(E))

clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp

void int_to_float() {
 
   Agg<float> f4 = {e};  // expected-error {{ cannot be narrowed }} expected-note {{silence}}
   Agg<double> f5 = {e};  // expected-error {{ cannot be narrowed }} expected-note {{silence}}
   Agg<long double> f6 = {e};  // expected-error {{ cannot be narrowed }} expected-note {{silence}}
 
   // Constants.
   Agg<float> f7 = {12345678};  // OK (exactly fits in a float)
   Agg<float> f8 = {EnumVal};  // OK
-  Agg<float> f9 = {123456789};  // expected-error {{ cannot be narrowed }} expected-note {{silence}}
+  Agg<float> f9 = {123456789};  // expected-error {{ cannot be narrowed }} expected-note {{silence}} expected-warning {{implicit conversion from 'int' to 'float' changes value}}
 
-  Agg<float> ce1 = { Convert<int>(123456789) }; // expected-error {{constant expression evaluates to 123456789 which cannot be narrowed to type 'float'}} expected-note {{silence}}
+  Agg<float> ce1 = { Convert<int>(123456789) }; // expected-error {{constant expression evaluates to 123456789 which cannot be narrowed to type 'float'}} expected-note {{silence}} expected-warning {{implicit conversion from 'int' to 'float' changes value}}
   Agg<double> ce2 = { ConvertVar<long long>() }; // expected-error {{non-constant-expression cannot be narrowed from type 'long long' to 'double'}} expected-note {{silence}}
 }
 
 // * from an integer type or unscoped enumeration type to an integer type that
 //   cannot represent all the values of the original type, except where the
 //   source is a constant expression and the actual value after conversion will
 //   fit into the target type and will produce the original value when converted
 //   back to the original type.

clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp

void int_to_float() {
 
   // Variables.  Yes, even though all char's will fit into any floating type.
   Agg<float> f1 = {c};  // expected-warning {{ cannot be narrowed }} expected-note {{silence}}
   Agg<double> f2 = {c};  // expected-warning {{ cannot be narrowed }} expected-note {{silence}}
   Agg<long double> f3 = {c};  // expected-warning {{ cannot be narrowed }} expected-note {{silence}}
 
   // Constants.
   Agg<float> f4 = {12345678};  // OK (exactly fits in a float)
-  Agg<float> f5 = {123456789};  // expected-warning {{ cannot be narrowed }} expected-note {{silence}}
+  Agg<float> f5 = {123456789};  // expected-warning {{ cannot be narrowed }} expected-note {{silence}} expected-warning {{implicit conversion from 'int' to 'float' changes value}}
 
-  Agg<float> ce1 = { Convert<int>(123456789) }; // expected-warning {{constant expression evaluates to 123456789 which cannot be narrowed to type 'float'}} expected-note {{silence}}
+  Agg<float> ce1 = { Convert<int>(123456789) }; // expected-warning {{constant expression evaluates to 123456789 which cannot be narrowed to type 'float'}} expected-note {{silence}} expected-warning {{implicit conversion from 'int' to 'float' changes value}}
   Agg<double> ce2 = { ConvertVar<long long>() }; // expected-warning {{non-constant-expression cannot be narrowed from type 'long long' to 'double'}} expected-note {{silence}}
 }
 
 // * from an integer type or unscoped enumeration type to an integer type that
 //   cannot represent all the values of the original type, except where the
 //   source is a constant expression and the actual value after conversion will
 //   fit into the target type and will produce the original value when converted
 //   back to the original type.

clang/test/Sema/conversion.c

 }
 
 void test8(int v) {
   takes_char(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_short(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
 
 void test9(long v) {
   takes_char(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_short(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v);  // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
 void test10(long long v) {
   takes_char(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_short(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v);  // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
 void test11(float v) {
   takes_char(v); // expected-warning {{implicit conversion turns floating-point number into integer}}
   takes_short(v); // expected-warning {{implicit conversion turns floating-point number into integer}}
   takes_int(v); // expected-warning {{implicit conversion turns floating-point number into integer}}
   takes_long(v); // expected-warning {{implicit conversion turns floating-point number into integer}}

clang/test/Sema/ext_vector_casts.c

static void splats(int i, long l, __uint128_t t, float f, double d) {
   vi = 1 + vi;
   vi = vi + 2.0; // expected-error {{cannot convert between vector values of different size}}
   vi = vi + 0xffffffff; // expected-warning {{implicit conversion changes signedness}}
   
   vl = l + vl; // expected-warning {{implicit conversion changes signedness}}
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0xffffffff;
+  vf = vf + 0xffffffff; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
-  
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }

clang/test/Sema/implicit-int-float-conversion.c

+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-int-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 222222;
+  double b = 222222222222L;
+
+  float ff = 222222222222L;    // expected-warning {{implicit conversion from 'long' to 'float' changes value from 222222222222 to 222222221312}}
+  float ffff = 222222222222UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from 222222222222 to 222222221312}}
+
+  long l = 222222222222L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+  float b = 222222222222L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from 222222222222 to 222222221312}}
+
+  float g = 22222222 + 22222222;
+  float c = 22222222 + 22222223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 44444445 to 44444444}}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+
+  double e = 0.0;
+  double f = i + e;
+}