Audit -Wtautological-type-limit-compare warnings and fix the ones that are true positives. |
||||
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()) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
,
Sep 6
https://chromium-review.googlesource.com/c/chromium/src/+/1211742 for dry run results.
,
Sep 6
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.
,
Sep 6
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.
,
Sep 6
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.
,
Sep 6
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.
,
Jan 11
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 |
||||
Comment 1 by wfh@chromium.org
, Sep 6Status: Available (was: Untriaged)