Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118383006:
Fix "conversion of size_t to unsigned int" warning (Closed)
|
Created:
4 years, 5 months ago by Rob Percival Modified:
4 years, 5 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix "conversion of size_t to unsigned int" warning
On Windows 64-bit builds, sizeof(size_t) != sizeof(unsigned):
net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data
BUG=612439
Committed: https://crrev.com/8c82b175a6fa0cc4edd1c6647fd77a7f44525a65
Cr-Commit-Position: refs/heads/master@{#404146}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Re-inline NsecRecordRdata::bitmap_length() and remove DCHECK #
Total comments: 2
Patch Set 3 : Addresses comments from Ryan #
Total comments: 3
Patch Set 4 : Addresses final comments from Ryan #Messages
Total messages: 29 (12 generated)
robpercival@chromium.org changed reviewers: + mmenke@chromium.org
PTAL - fixes Windows-breaking compilation warning.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robpercival@chromium.org changed reviewers: + rsleevi@chromium.org - mmenke@chromium.org
Description was changed from ========== Fix "conversion of size_t to unsigned int" warning net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data BUG=612439 ========== to ========== Fix "conversion of size_t to unsigned int" warning On Windows 64-bit builds, sizeof(size_t) != sizeof(unsigned): net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data BUG=612439 ==========
To motivations: Why are you seeing this? We suppress the warnings in //net https://cs.chromium.org/chromium/src/net/net.gyp?rcl=0&l=359 https://codereview.chromium.org/2118383006/diff/20001/net/dns/record_rdata.cc File net/dns/record_rdata.cc (right): https://codereview.chromium.org/2118383006/diff/20001/net/dns/record_rdata.cc... net/dns/record_rdata.cc:263: DCHECK_LE(bitmap_.size(), 32u); It's unclear if you added this DCHECK as documentary purposes (this is guaranteed) or as a guard against developer mistake ("size should be less than 32") If the latter, it's a bad guard - because this is handling untrusted data, there's no way the caller could validate/preprocess the data to make sure that this was true. If the former, it's not sufficient documentation - because I still had to try to figure out which it was, and why it was safe to document this. (For the record, it's because of line 241) The cost of the presumed documentation is that it forces you to de-inline this, and I'm not sure that's worth it.
On 2016/07/06 15:28:58, Ryan Sleevi wrote: > To motivations: Why are you seeing this? We suppress the warnings in //net > > https://cs.chromium.org/chromium/src/net/net.gyp?rcl=0&l=359 > > https://codereview.chromium.org/2118383006/diff/20001/net/dns/record_rdata.cc > File net/dns/record_rdata.cc (right): > > https://codereview.chromium.org/2118383006/diff/20001/net/dns/record_rdata.cc... > net/dns/record_rdata.cc:263: DCHECK_LE(bitmap_.size(), 32u); > It's unclear if you added this DCHECK as documentary purposes (this is > guaranteed) or as a guard against developer mistake ("size should be less than > 32") > > If the latter, it's a bad guard - because this is handling untrusted data, > there's no way the caller could validate/preprocess the data to make sure that > this was true. > > If the former, it's not sufficient documentation - because I still had to try to > figure out which it was, and why it was safe to document this. (For the record, > it's because of line 241) > > The cost of the presumed documentation is that it forces you to de-inline this, > and I'm not sure that's worth it. As you discovered, it's the former (documentation). What's your preference for resolving this? a) Remove DCHECK and re-inline the function. b) Add a comment providing further explanation. c) Replace the DCHECK with a comment and re-line the function. I've provisionally gone for option c, but I did prefer having the DCHECK for peace of mind. Any opposition to adding it to the inlined function or are you happier with it as-is?
On 2016/07/06 15:56:23, Rob Percival wrote: > On 2016/07/06 15:28:58, Ryan Sleevi wrote: > > To motivations: Why are you seeing this? We suppress the warnings in //net > > > > https://cs.chromium.org/chromium/src/net/net.gyp?rcl=0&l=359 > > > > https://codereview.chromium.org/2118383006/diff/20001/net/dns/record_rdata.cc > > File net/dns/record_rdata.cc (right): > > > > > https://codereview.chromium.org/2118383006/diff/20001/net/dns/record_rdata.cc... > > net/dns/record_rdata.cc:263: DCHECK_LE(bitmap_.size(), 32u); > > It's unclear if you added this DCHECK as documentary purposes (this is > > guaranteed) or as a guard against developer mistake ("size should be less than > > 32") > > > > If the latter, it's a bad guard - because this is handling untrusted data, > > there's no way the caller could validate/preprocess the data to make sure that > > this was true. > > > > If the former, it's not sufficient documentation - because I still had to try > to > > figure out which it was, and why it was safe to document this. (For the > record, > > it's because of line 241) > > > > The cost of the presumed documentation is that it forces you to de-inline > this, > > and I'm not sure that's worth it. > > As you discovered, it's the former (documentation). What's your preference for > resolving this? > a) Remove DCHECK and re-inline the function. > b) Add a comment providing further explanation. > c) Replace the DCHECK with a comment and re-line the function. > > I've provisionally gone for option c, but I did prefer having the DCHECK for > peace of mind. Any opposition to adding it to the inlined function or are you > happier with it as-is? There's another option, come to think of it. bitmap_length() could return size_t and be ignorant of the fact that the bitmap can only be up to 256 bits long. That's closest to the existing implementation and is the smallest possible change to fix the compilation warning.
I'm fine with Option C, mod nits. My big problem with the DCHECK isn't that it doesn't serve valuable documentation - like, knowing that I can safely reason about that static_cast<> *is* valuable - just that it's hard to distinguish them from when they're being used as protection. That is, the difference between "developer-did-something-wrong CHECKs"/"documentation CHECKs" and "Defense-against-the-dark-arts CHECKs" I'm not fundamentally opposed to de-inlining and adding the DCHECK, since we don't have perf numbers to measure and even in the header the inline is at best a hint to the compiler and we can hope our compilers will continue to get smarter to avoid the need to micro-optimize, but I did want to make sure that the reasoning for it is clear. Ideally, you'd also add a unittest to cover that 'invalid' input case, which ensures it is rejected during the Create() call, and then it's even more useful as a "documentation-and-developer-defense" - the unittest makes sure Create() isn't changed, and the documentation lets the developer know it's intentional. I could even live with inline + DCHECK https://codereview.chromium.org/2118383006/diff/40001/net/dns/record_rdata.h File net/dns/record_rdata.h (right): https://codereview.chromium.org/2118383006/diff/40001/net/dns/record_rdata.h#... net/dns/record_rdata.h:202: // bytes long (this is verified by Create()). This is the sort of comment that makes me uneasy in the header (the parenthetical), because that talks about implementation. // This will be between 8 and 256 bits long, per RFC [blah] Section [blah]
On 2016/07/06 16:29:03, Ryan Sleevi wrote: > I'm fine with Option C, mod nits. > > My big problem with the DCHECK isn't that it doesn't serve valuable > documentation - like, knowing that I can safely reason about that static_cast<> > *is* valuable - just that it's hard to distinguish them from when they're being > used as protection. That is, the difference between > "developer-did-something-wrong CHECKs"/"documentation CHECKs" and > "Defense-against-the-dark-arts CHECKs" > > I'm not fundamentally opposed to de-inlining and adding the DCHECK, since we > don't have perf numbers to measure and even in the header the inline is at best > a hint to the compiler and we can hope our compilers will continue to get > smarter to avoid the need to micro-optimize, but I did want to make sure that > the reasoning for it is clear. > > Ideally, you'd also add a unittest to cover that 'invalid' input case, which > ensures it is rejected during the Create() call, and then it's even more useful > as a "documentation-and-developer-defense" - the unittest makes sure Create() > isn't changed, and the documentation lets the developer know it's intentional. > > I could even live with inline + DCHECK > > https://codereview.chromium.org/2118383006/diff/40001/net/dns/record_rdata.h > File net/dns/record_rdata.h (right): > > https://codereview.chromium.org/2118383006/diff/40001/net/dns/record_rdata.h#... > net/dns/record_rdata.h:202: // bytes long (this is verified by Create()). > This is the sort of comment that makes me uneasy in the header (the > parenthetical), because that talks about implementation. > > // This will be between 8 and 256 bits long, per RFC [blah] Section [blah] Done (inline + DCHECK, unittests). As for the motivation, it was this build failure that got past the CQ buildbots: http://build.chromium.org/p/chromium/builders/Win%20x64/builds/2184. I think it's because record_rdata.h is included by components/certificate_transparency/log_dns_client.cc, which doesn't disable that warning.
https://codereview.chromium.org/2118383006/diff/40001/net/dns/record_rdata.h File net/dns/record_rdata.h (right): https://codereview.chromium.org/2118383006/diff/40001/net/dns/record_rdata.h#... net/dns/record_rdata.h:202: // bytes long (this is verified by Create()). On 2016/07/06 16:29:03, Ryan Sleevi wrote: > This is the sort of comment that makes me uneasy in the header (the > parenthetical), because that talks about implementation. > > // This will be between 8 and 256 bits long, per RFC [blah] Section [blah] I had the same feeling when I wrote it. I wasn't convinced just referring to the RFC would be sufficient though, since what the RFC allows and what comes over the network can be very different. I wanted to say something closer to what the DCHECK was intended to express - that it is an internal guarantee.
LGTM % nits https://codereview.chromium.org/2118383006/diff/60001/net/dns/record_rdata_un... File net/dns/record_rdata_unittest.cc (right): https://codereview.chromium.org/2118383006/diff/60001/net/dns/record_rdata_un... net/dns/record_rdata_unittest.cc:200: std::unique_ptr<NsecRecordRdata> record_obj; STYLE: Keep this scoped closer to where it's used - per https://google.github.io/styleguide/cppguide.html#Local_Variables That is, line 211 std::unique_ptr<NsecRecordRdata> record_obj = NSecRecordRdata... ASSERT_FALSE(record_obj); https://codereview.chromium.org/2118383006/diff/60001/net/dns/record_rdata_un... net/dns/record_rdata_unittest.cc:212: ASSERT_EQ(nullptr, record_obj); ASSERT_FALSE(record_obj) https://codereview.chromium.org/2118383006/diff/60001/net/dns/record_rdata_un... net/dns/record_rdata_unittest.cc:233: ASSERT_EQ(nullptr, record_obj); ASSERT_FALSE(record_obj)
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2118383006/#ps80001 (title: "Addresses final comments from Ryan")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix "conversion of size_t to unsigned int" warning On Windows 64-bit builds, sizeof(size_t) != sizeof(unsigned): net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data BUG=612439 ========== to ========== Fix "conversion of size_t to unsigned int" warning On Windows 64-bit builds, sizeof(size_t) != sizeof(unsigned): net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data BUG=612439 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix "conversion of size_t to unsigned int" warning On Windows 64-bit builds, sizeof(size_t) != sizeof(unsigned): net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data BUG=612439 ========== to ========== Fix "conversion of size_t to unsigned int" warning On Windows 64-bit builds, sizeof(size_t) != sizeof(unsigned): net\dns\record_rdata.h(201): warning C4267: 'return': conversion from 'size_t' to 'unsigned int', possible loss of data BUG=612439 Committed: https://crrev.com/8c82b175a6fa0cc4edd1c6647fd77a7f44525a65 Cr-Commit-Position: refs/heads/master@{#404146} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8c82b175a6fa0cc4edd1c6647fd77a7f44525a65 Cr-Commit-Position: refs/heads/master@{#404146} |
