This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ziangwan on Jul 12 2019, 2:17 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ziangwan created this revision.Jul 12 2019, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 2:17 PM
ziangwan edited the summary of this revision. (Show Details)Jul 12 2019, 2:21 PM
jfb added a comment.Jul 12 2019, 2:23 PM

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

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

ziangwan updated this revision to Diff 209591.Jul 12 2019, 2:25 PM

Removed patch.patch.

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

Thanks for the patch @ziangwan !

clang/include/clang/Basic/DiagnosticSemaKinds.td
3245 ↗(On Diff #209587)

may lose or just loses.

clang/lib/Sema/SemaChecking.cpp
11404 ↗(On Diff #209587)

significand

11412 ↗(On Diff #209587)

End the sentence w/ punctuation.

11417 ↗(On Diff #209587)

should this use SourceBT rather than TargetBT?

11420 ↗(On Diff #209587)

accuracy

:set spell if you're using vim

11421 ↗(On Diff #209587)

Maybe negate this conditional and return early?

11437 ↗(On Diff #209587)

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

etalvala added inline comments.
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?

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

[Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss
ziangwan added a comment.EditedJul 12 2019, 2:40 PM

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 retitled this revision from Allow Clang -Wconversion, -Wimplicit-float-conversion warns about integer type -> floating point type implicit conversion precision loss. to [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss.Jul 12 2019, 2:43 PM
ziangwan updated this revision to Diff 209611.Jul 12 2019, 3:00 PM

Fix spelling / comment / naming issues.

ziangwan marked 6 inline comments as done.Jul 12 2019, 3:03 PM
ziangwan added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3245 ↗(On Diff #209587)

"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 ↗(On Diff #209587)

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

ziangwan marked 2 inline comments as done.Jul 12 2019, 3:05 PM
ziangwan added inline comments.
clang/test/Sema/implicit-float-conversion.c
29 ↗(On Diff #209591)

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

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 added inline comments.Jul 12 2019, 3:11 PM
clang/lib/Sema/SemaChecking.cpp
11430 ↗(On Diff #209611)

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?

clang/include/clang/Basic/DiagnosticSemaKinds.td
3245 ↗(On Diff #209587)

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

clang/lib/Sema/SemaChecking.cpp
11416 ↗(On Diff #209611)

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 ↗(On Diff #209611)

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

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 updated this revision to Diff 209710.Jul 13 2019, 6:59 PM

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 marked 9 inline comments as done.Jul 13 2019, 7:09 PM
ziangwan added inline comments.
clang/lib/Sema/SemaChecking.cpp
11430 ↗(On Diff #209611)

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 ↗(On Diff #209611)

Test cases are added

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 accepted this revision.Jul 15 2019, 10:39 AM

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 ↗(On Diff #209710)

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.

This revision is now accepted and ready to land.Jul 15 2019, 10:39 AM

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

scanon requested changes to this revision.Jul 15 2019, 10:50 AM
scanon added a subscriber: scanon.
scanon added inline comments.
clang/lib/Sema/SemaChecking.cpp
11429 ↗(On Diff #209710)

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.

This revision now requires changes to proceed.Jul 15 2019, 10:50 AM
ziangwan updated this revision to Diff 209928.EditedJul 15 2019, 12:02 PM
ziangwan marked an inline comment as done.
ziangwan edited the summary of this revision. (Show Details)

Update diff.

  1. Adopt scanon's comment. Check the opStatus of int literal->float conversion to determine precision loss. All tests passed.
ziangwan marked 2 inline comments as done.Jul 15 2019, 12:04 PM
ziangwan marked an inline comment as done.Jul 16 2019, 10:41 AM
ziangwan added inline comments.
clang/lib/Sema/SemaChecking.cpp
11429 ↗(On Diff #209710)

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

scanon accepted this revision.Jul 16 2019, 10:49 AM

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.

This revision is now accepted and ready to land.Jul 16 2019, 10:49 AM

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

xbolva00 added a comment.EditedJul 22 2019, 2:55 PM

@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 accepted this revision.Jul 22 2019, 3:41 PM
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 ↗(On Diff #209587)

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.

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

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 marked 3 inline comments as done.Jul 22 2019, 4:09 PM

@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 should warn in that case even if GCC does not warn.

ziangwan updated this revision to Diff 211245.Jul 22 2019, 8:19 PM

Update diff

  1. Fix trailing whitespaces.
ziangwan marked an inline comment as done.Jul 22 2019, 8:34 PM

Final review ping.

aaron.ballman requested changes to this revision.Jul 23 2019, 5:03 AM

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

Strong +1.

Final review ping.

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

This revision now requires changes to proceed.Jul 23 2019, 5:03 AM
jfb requested changes to this revision.Jul 23 2019, 9:21 AM

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.

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.

In D64666#1597627, @jfb 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.

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.

@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 added inline comments.Jul 23 2019, 10:39 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3279 ↗(On Diff #211245)

Drop ‘DefaultIgnore‘ here

ziangwan marked an inline comment as done.Jul 23 2019, 3:38 PM
ziangwan added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3279 ↗(On Diff #211245)

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

ziangwan updated this revision to Diff 211381.Jul 23 2019, 4:37 PM

Update diff.

  1. Make "definitely lose" warning on by default.
ziangwan marked an inline comment as done.Jul 23 2019, 4:37 PM
jfb added a comment.Jul 23 2019, 4:52 PM

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.

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 added a comment.Jul 24 2019, 10:18 AM
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.

In D64666#1599544, @jfb 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 updated this revision to Diff 211617.Jul 24 2019, 3:41 PM

Update diff.

  1. Make the two warnings controlled by -Wimplicit-int-float-conversion, which is a subset of -Wimplicit-float-conversion.
jfb accepted this revision.Jul 24 2019, 3:48 PM

Great, thanks!

xbolva00 accepted this revision.Jul 24 2019, 3:49 PM

Looks ok

clang/include/clang/Basic/DiagnosticGroups.td
739 ↗(On Diff #211617)

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

aaron.ballman accepted this revision.Jul 24 2019, 4:00 PM

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

This revision is now accepted and ready to land.Jul 24 2019, 4:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 5:32 PM
phosek added a subscriber: phosek.Jul 24 2019, 6:34 PM

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 added a comment.EditedJul 24 2019, 7:43 PM

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 reopened this revision.Jul 24 2019, 8:11 PM

Reverted in r366979.

This revision is now accepted and ready to land.Jul 24 2019, 8:11 PM
phosek requested changes to this revision.Jul 24 2019, 8:11 PM
This revision now requires changes to proceed.Jul 24 2019, 8:11 PM
ziangwan updated this revision to Diff 211666.Jul 24 2019, 9:11 PM

Update diff.

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

Okay?

MaskRay added a subscriber: MaskRay.EditedJul 24 2019, 9:28 PM

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 resigned from this revision.EditedJul 25 2019, 1:10 AM

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 added a comment.EditedJul 25 2019, 10:32 AM

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 updated this revision to Diff 212001.Jul 26 2019, 1:51 PM

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.
ziangwan marked an inline comment as done.Jul 26 2019, 1:51 PM
xbolva00 accepted this revision.Jul 26 2019, 2:03 PM

Nice simple fix :) thanks

xbolva00 added inline comments.Jul 26 2019, 2:23 PM
clang/lib/Sema/SemaChecking.cpp
11653 ↗(On Diff #212001)

Typo

11654 ↗(On Diff #212001)

C++11

(But please wait for @aaron.ballman)

ziangwan updated this revision to Diff 212015.Jul 26 2019, 3:14 PM

Fix typos.

ziangwan marked 2 inline comments as done.Jul 26 2019, 3:15 PM
aaron.ballman added inline comments.Jul 28 2019, 8:14 AM
clang/lib/Sema/SemaChecking.cpp
11656 ↗(On Diff #212015)

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

ziangwan marked an inline comment as done.Jul 29 2019, 10:35 AM
ziangwan added inline comments.
clang/lib/Sema/SemaChecking.cpp
11656 ↗(On Diff #212015)

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 accepted this revision.Jul 29 2019, 10:52 AM

LGTM!

clang/lib/Sema/SemaChecking.cpp
11656 ↗(On Diff #212015)

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 marked an inline comment as done.Jul 30 2019, 11:14 AM
ziangwan added inline comments.
clang/lib/Sema/SemaChecking.cpp
11656 ↗(On Diff #212015)

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 added inline comments.Jul 30 2019, 12:03 PM
clang/lib/Sema/SemaChecking.cpp
11656 ↗(On Diff #212015)

Agreed. :-)

MaskRay accepted this revision.Jul 30 2019, 11:17 PM
ziangwan marked 3 inline comments as done.Jul 31 2019, 4:12 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 31 2019, 5:16 PM
This revision was automatically updated to reflect the committed changes.
ziangwan added a comment.EditedJul 31 2019, 5:16 PM

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

Buildbot failed for x86_64 target. Fix in progress.

ziangwan retitled this revision from [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss to [Sema] Enable -Wimplicit-int-float-conversion for integral to floating point precision loss.Aug 2 2019, 2:29 PM
ziangwan added a comment.EditedAug 2 2019, 2:33 PM

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.

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.

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 added a comment.EditedAug 4 2019, 12:01 PM

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.

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

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