(Also, we should check if there are other warnings that are similarly inconsistent.)

> (Also, we should check if there are other warnings that are similarly inconsistent.)

Oh, here’s the problem!

https://codesearch.chromium.org/search/?q=wno-inconsistent&sq=package:chromium&type=cs

JUST KIDDING.

Hi Sylvain, can you take a look?

This is because clang Xcode uses different default when enabling all warnings. Just need to disable this particular warning on clang Xcode.

Looking more into this, I think the CL is incorrect and this should have been reverted. Not sure what need to be done here. Do we want to enable this check on all bots instead?

hans: WDYT?

"Looking more into this, I think the CL is incorrect and this should have been reverted."

This is arguable. The warning is a variation of "this if statement is always true/false" - and in fact that was the desired behavior here to avoid platform-specific ifdefs. Even the modified code is a case where we check some_int < max_int which should also be always true (but this warning doesn't trigger).

Anyway, I'm OK with this being enabled everywhere as well - my point is we should ensure that there's consistency between different builds. Enabling everywhere is probably a bit more work since you likely have to fix existing code that currently would trigger this but that we don't build on iOS.

I agree that we should be consistent. I incorrectly expected the work to be trivial since ios-device-xcode-clang had been using that flag for some time, but forgot that not everything is built on iOS.

Will create a CL to disable the warning when using xcode_clang.

Thanks! Can you also check if there are other warnings like this? (e.g. compare the list of warnings we enable on the two builds?)

I think the issue is that -Wextra/-Wall do not include the same warnings with the two version of the compiler. I have not found any automated way to get the list of all enabled warnings.

The -verbose flag does not seems to give the detailed list of warnings. Comparing the list by hand manually is going to be painful.

You can work out the warnings by comparing tools/clang/include/clang/Basic/DiagnosticGroups.td, but I don’t think that’s what’s happening here. -Wall enables -Wtautological-constant-out-of-range-compare in both Chrome’s trunk clang and in Apple’s Xcode clang. (And if you’re using Xcode 9, Apple hasn’t published their clang source yet at https://opensource.apple.com/.)

mark@mela zsh% cat t.cc 
#include <limits>

bool F(unsigned short s) {
  return s > std::numeric_limits<int>::max();
}
mark@mela zsh% clang++ -std=c++14 -Wall -c t.cc -o t
t.cc:4:12: warning: comparison of constant 2147483647 with expression of type
      'unsigned short' is always false
      [-Wtautological-constant-out-of-range-compare]
  return s > std::numeric_limits<int>::max();
         ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
mark@mela zsh% crclang++ -std=c++14 -Wall -c t.cc -o t
t.cc:4:12: warning: comparison of constant 2147483647 with expression of type
      'unsigned short' is always false
      [-Wtautological-constant-out-of-range-compare]
  return s > std::numeric_limits<int>::max();
         ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

These two compilers are:

mark@mela zsh% clang++ -v
Apple LLVM version 9.0.0 (clang-900.0.38)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
mark@mela zsh% crclang++ -v
clang version 6.0.0 (trunk 317263)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: /Users/mark/bin

(Apple clang 900.0.38 is from Xcode 9.1 9B55.)

I'm still catching up here, but yes, my impression was that we have -Wtautological-constant-out-of-range-compare enabled both with regular Clang and xcode-clang.

But then the question is of course how the CL got through the CQ. It's possible that the warning has changed somehow in upstream Clang, so maybe there's a bug somewhere, or maybe there's something funky going on with the build config.

#12: given https://clang.llvm.org/docs/DiagnosticsReference.html#wtautological-constant-compare and https://codesearch.chromium.org/chromium/src/build/config/compiler/BUILD.gn?rcl=8ec7714af9698919b2f8b40f3f23e1d8130fb464&l=1304, it looks like we're disabling it outside of xcode-clang?

Ah, now I remember. -Wtautological-constant-compare is a new flag, encompasses -Wtautological-constant-out-of-range-compare et al. which were enhanced at the same time, and so we had to turn them off because we're not clean.

This is a mess :-/

Mark, can you check if your xcode recognizes the -Wtautological-constant-compare option?

I'm surprised it warns about the code here because that was only added in October (r315614), but maybe they cherry-picked it into their release branch, or had it there earlier.

It does not.

mark@mela zsh% clang++ -Wtautological-constant-compare -x c++ -c /dev/null -o t.o
warning: unknown warning option '-Wtautological-constant-compare'; did you mean
      '-Wtautological-pointer-compare'? [-Wunknown-warning-option]
1 warning generated.
mark@mela zsh% clang++ -v
Apple LLVM version 9.0.0 (clang-900.0.38)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
mark@mela zsh% xcodebuild -version
Xcode 9.1
Build version 9B55

Thanks.

Okay, I think this is what we need: https://chromium-review.googlesource.com/#/c/chromium/src/+/764035

Hans, the warning that’s being triggered is -Wtautological-constant-out-of-range-compare, which has been in clang since r164316 (actually r164143), 2012.

Oh, I see. -Wno-tautological-constant-compare creates an accidental mess.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e83806f7408da1ea396f2eeae5da32905142d438

commit e83806f7408da1ea396f2eeae5da32905142d438
Author: Hans Wennborg <hans@chromium.org>
Date: Fri Nov 10 23:20:59 2017

Clang: re-enable -Wtautological-constant-out-of-range-compare

It was unintentionally disabled as part of disabling the new Clang
warning -Wtautological-constant-compare.

Bug:  767059 ,  783204 
Change-Id: I621f002f319cb395ecc30ca7ff9f132cefe0e5d4
Reviewed-on: https://chromium-review.googlesource.com/764035
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515739}
[modify] https://crrev.com/e83806f7408da1ea396f2eeae5da32905142d438/build/config/compiler/BUILD.gn

-Wtautological-constant-out-of-range-compare is now re-enabled on all the bots. Sorry for the mess.

Thanks!