New issue
Advanced search Search tips

Issue 881479 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Audit -Wtautological-type-limit-compare warnings and fix the ones that are true positives.

Project Member Reported by wfh@chromium.org, Sep 6

Issue description

we should consider enabling -Wtautological-type-limit-compare

It would find stuff like

if (static_cast<int>(source.size()) > std::numeric_limits<int>::max())

gives warnings like:

../../chrome/install_static/install_util.cc(687,39):  warning: result of comparison 'int' > 2147483647 is always false [-Wtautological-type-limit-compare]
      static_cast<int>(source.size()) > std::numeric_limits<int>::max()) {
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
Cc: thakis@chromium.org grt@chromium.org
Status: Available (was: Untriaged)
This is likely a smaller job than other warnings, but I haven't actually tried a full compile with this enabled.
Pretty sure we don't want this, because it's target arch dependent. See also http://llvm.org/viewvc/llvm-project?view=revision&revision=322901

In general (with the big exception being -Wconversion), the default warning set you get from -Wall is supposed to be all the warnings you really want.
Summary: Audit -Wtautological-type-limit-compare warnings and fix the ones that are true positives. (was: enable -Wtautological-type-limit-compare)
yes, I agree with #3 but if the warning appears in both 32-bit and 64-bit it's probably a false positive.

e.g. the one in #0 is almost certainly wrong code, and others exist e.g. 

../../net/third_party/quic/core/quic_framer.cc(3396,28):  warning: result of comparison 'const quic::QuicPacketLength' (aka 'const unsigned short') > 65535 is always false [-Wtautological-type-limit-compare]
    if ((frame.data_length > std::numeric_limits<uint16_t>::max()) ||
         ~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I suppose I'll rename the bug to more accurately represent what is wanted here.
and of course, typo in #4, I mean if it appears in one arch (64 or 32) and not the other then it's probably a false positive...

So it would be a matter of grepping through the build output from the CL in #2 and finding common ones, and auditing those parts of code.
Labels: -Pri-2 Pri-3
very quick back-of-a-shell results (for just Windows)

Comparing a 32-bit build (win_chromium_compile_dbg_ng) with a 32-bit build (win10_chromium_x64_rel_ng)

wget https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8936105917384170752/+/steps/compile__with_patch_/0/stdout -O win_chromium_compile_dbg_ng.log
wget https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8936105917384170784/+/steps/compile__with_patch_/0/stdout -O win10_chromium_x64_rel_ng.log

grep 'warning: result of comparison' win_chromium_compile_dbg_ng.log  |sort|uniq > 32.txt
grep 'warning: result of comparison' win10_chromium_x64_rel_ng.log  |sort|uniq > 64.txt

comm -12 32.txt 64.txt

../../chrome/browser/extensions/api/downloads/downloads_api.cc(211,14):  warning: result of comparison 'download::DownloadDangerType' < 0 is always false [-Wtautological-type-limit-compare]
../../chrome/browser/extensions/api/downloads/downloads_api.cc(229,13):  warning: result of comparison 'download::DownloadItem::DownloadState' < 0 is always false [-Wtautological-type-limit-compare]
../../chrome/install_static/install_util.cc(669,39):  warning: result of comparison 'int' > 2147483647 is always false [-Wtautological-type-limit-compare]
../../chrome/install_static/install_util.cc(687,39):  warning: result of comparison 'int' > 2147483647 is always false [-Wtautological-type-limit-compare]
../../components/autofill/core/browser/autofill_field.cc(54,12):  warning: result of comparison 'autofill::ServerFieldType' >= 0 is always true [-Wtautological-type-limit-compare]
../../components/feed/core/feed_networking_host_unittest.cc(85,14):  warning: result of comparison 'net::HttpStatusCode' >= 0 is always true [-Wtautological-type-limit-compare]
../../components/nacl/renderer/trusted_plugin_channel.cc(45,19):  warning: result of comparison 'NaClErrorCode' < 0 is always false [-Wtautological-type-limit-compare]
../../components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc(186,23):  warning: result of comparison 'net::HttpStatusCode' >= 0 is always true [-Wtautological-type-limit-compare]
../..\content/common/media/media_devices.h(55,15):  warning: result of comparison 'content::MediaDeviceType' >= 0 is always true [-Wtautological-type-limit-compare]
../../content/public/common/referrer.cc(30,33):  warning: result of comparison 'blink::WebReferrerPolicy' < 0 is always false [-Wtautological-type-limit-compare]
../../content/renderer/media/stream/media_stream_constraints_util_sets.cc(47,17):  warning: result of comparison 'long' > 2147483647 is always false [-Wtautological-type-limit-compare]
../../mojo/core/shared_buffer_dispatcher.cc(276,14):  warning: result of comparison 'uint64_t' (aka 'unsigned long long') > 18446744073709551615 is always false [-Wtautological-type-limit-compare]
../../mojo/core/shared_buffer_dispatcher.cc(278,17):  warning: result of comparison 'uint64_t' (aka 'unsigned long long') > 18446744073709551615 is always false [-Wtautological-type-limit-compare]
../../net/dns/dns_util.cc(187,12):  warning: result of comparison 'NetworkChangeNotifier::ConnectionType' < 0 is always false [-Wtautological-type-limit-compare]
../../net/ftp/ftp_server_type_histograms.cc(22,12):  warning: result of comparison 'net::FtpServerType' >= 0 is always true [-Wtautological-type-limit-compare]
../../net/third_party/quic/core/quic_framer.cc(3396,28):  warning: result of comparison 'const quic::QuicPacketLength' (aka 'const unsigned short') > 65535 is always false [-Wtautological-type-limit-compare]
../../net/third_party/quic/test_tools/quic_test_utils.cc(685,32):  warning: result of comparison 'const char' <= 127 is always true [-Wtautological-type-limit-compare]
../../ppapi/proxy/video_decoder_resource.cc(128,15):  warning: result of comparison 'PP_VideoProfile' < 0 is always false [-Wtautological-type-limit-compare]
../../remoting/test/app_remoting_service_urls.cc(29,31):  warning: result of comparison 'remoting::test::ServiceEnvironment' >= 0 is always true [-Wtautological-type-limit-compare]
../../third_party/angle/src/common/utilities.cpp(791,28):  warning: result of comparison 'unsigned long' <= 4294967295 is always true [-Wtautological-type-limit-compare]
../../third_party/blink/renderer/platform/text/text_break_iterator.cc(267,18):  warning: result of comparison 'ULineBreak' >= 0 is always true [-Wtautological-type-limit-compare]
../../third_party/blink/renderer/platform/text/text_break_iterator.cc(267,70):  warning: result of comparison 'ULineBreak' >= 0 is always true [-Wtautological-type-limit-compare]
../../ui/gfx/color_utils_unittest.cc(246,51):  warning: result of comparison 'SkAlpha' (aka 'unsigned char') <= 255 is always true [-Wtautological-type-limit-compare]
../../ui/native_theme/native_theme_win.cc(1892,34):  warning: result of comparison 'ui::NativeThemeWin::ThemeName' < 0 is always false [-Wtautological-type-limit-compare]

Note: the two mojo/core/shared_buffer_dispatcher.cc are actual false positives, because they are a nacl64 build on 32-bit.

tbh looks like a lot of chaff (but haven't looked closely), maybe the install_util.cc one is the only actual real issue.
Status: Untriaged (was: Available)
Available, but no owner or component? Please find a component, as no one will ever find this without one.

Sign in to add a comment