Fix style issue.

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?

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

Fixed.

Buildbot failed for x86_64 target. Fix in progress.

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

Fix typos.

Update diff.

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

Update diff.

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.

--

Update diff.

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.

Update diff.

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.

Final review ping.

Update diff

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.

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

Update diff.

Update diff.

Update diff:

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’?

Fix spelling / comment / naming issues.

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.

Removed patch.patch.

Update diff:

1. Have a workaround inside top-level CMakeLists.txt to set TARGET_OS to Android.
2. Test against TARGET_OS instead of TEST_SUITE_REMOTE_CLIENT in the build system.
3. lit.site.cfg.in: remove the double quote around @TEST_SUITE_PROFILE_GENERATE@.

LGTM

Hey @MaskRay , can you explain why we should remove --print-supported-cpus. There already are similar options in clang such as --print-effective-triple and --print-libgcc-file-name. On the other hand, I almost never see an option goes like -xxx=?. Also, why can't we support both since they are just aliases to each other?

Reviewed at https://reviews.llvm.org/D63105. Forgot to link it. Sorry.

This option is useful but may be hard to discover. Will something like -march=? and -mtune=? make the feature more discoverable?

That's a good idea. Currently, my change has nothing to do with -march or -mtune. I might work on that later.

@dyung I pushed another commit to fix the test. Let me know if the problem persists.

I am working on fixing the test file right now. I will add REQUIRES: x86-registered-target and REQUIRES: arm-registered-target before each test correspondingly.

Committed. SVN Revision: 363464.

Adjust the docs: remove the redundant -mcpu flag. Add an explanation for --print-supported-cpus.
Replace one manual delete with std::unique_ptr.
Use None for an llvm::Optional value.
Add one more test case.
Adjust the message printout to include the usage of -mtune.

Add full context. Change the name of the static variable from "Printed" to "PrintOnce". Add a newline for the test file.