Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1662683002:
Fix compile error for gn build of mini_installer (Closed)
DescriptionFix compile error for gn build of mini_installer
Invalid conversion from size_t to DWORD on line 529.
BUG=82385
(not sure that bug is relevant but https://codereview.chromium.org/1580673004/
made me think it was...)
Committed: https://crrev.com/e502a2e82b175f0424b6cd98a9bf84368ff960e6
Cr-Commit-Position: refs/heads/master@{#373261}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nullptr -> NULL #Messages
Total messages: 23 (10 generated)
Description was changed from ========== Fix compile error for gn build of mini_installer BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ========== to ========== Fix compile error for gn build of mini_installer BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ==========
Description was changed from
==========
Fix compile error for gn build of mini_installer
BUG=82385 (not sure that bug is relevant but
https://codereview.chromium.org/1580673004/
made me think it was...)
==========
to
==========
Fix compile error for gn build of mini_installer
BUG=82385
(not sure that bug is relevant but https://codereview.chromium.org/1580673004/
made me think it was...)
==========
Description was changed from ========== Fix compile error for gn build of mini_installer BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ========== to ========== Fix compile error for gn build of mini_installer BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ==========
gab@chromium.org changed reviewers: + grt@chromium.org
Greg PTAL
On 2016/02/02 21:35:19, gab wrote: > Greg PTAL Should the description be about the clang build rather than the GN build? What exactly was the compile error?
lgtm w/ nullptr -> NULL switch https://codereview.chromium.org/1662683002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1662683002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:530: static_cast<DWORD>(volume.capacity())) && ah, the size_t -> DWORD conversion was the problem? makes sense. https://codereview.chromium.org/1662683002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:531: ::GetVolumeInformation(volume.get(), nullptr, 0, nullptr, nullptr, we have to stick with C++-03 for a little longer in mini_installer. please revert to using NULL rather than nullptr.
Description was changed from ========== Fix compile error for gn build of mini_installer BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ========== to ========== Fix compile error for gn build of mini_installer Invalid conversion from size_t to DWORD on line 529. BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ==========
Thanks, done. https://codereview.chromium.org/1662683002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1662683002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:530: static_cast<DWORD>(volume.capacity())) && On 2016/02/03 14:54:11, grt wrote: > ah, the size_t -> DWORD conversion was the problem? makes sense. Right, sorry forgot to add that to description (and now down 11000 targets to rebuild before I can repro error because I was playing with my repo..) https://codereview.chromium.org/1662683002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:531: ::GetVolumeInformation(volume.get(), nullptr, 0, nullptr, nullptr, On 2016/02/03 14:54:11, grt wrote: > we have to stick with C++-03 for a little longer in mini_installer. please > revert to using NULL rather than nullptr. Done.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/1662683002/#ps20001 (title: "nullptr -> NULL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1662683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1662683002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix compile error for gn build of mini_installer Invalid conversion from size_t to DWORD on line 529. BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ========== to ========== Fix compile error for gn build of mini_installer Invalid conversion from size_t to DWORD on line 529. BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix compile error for gn build of mini_installer Invalid conversion from size_t to DWORD on line 529. BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) ========== to ========== Fix compile error for gn build of mini_installer Invalid conversion from size_t to DWORD on line 529. BUG=82385 (not sure that bug is relevant but https://codereview.chromium.org/1580673004/ made me think it was...) Committed: https://crrev.com/e502a2e82b175f0424b6cd98a9bf84368ff960e6 Cr-Commit-Position: refs/heads/master@{#373261} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e502a2e82b175f0424b6cd98a9bf84368ff960e6 Cr-Commit-Position: refs/heads/master@{#373261}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
what bot was this on? the clang/win bots look happy
Message was sent while issue was closed.
On 2016/02/03 17:23:14, Nico wrote: > what bot was this on? the clang/win bots look happy This was locally, when compiling mini_installer from a GN build (doesn't happen from GYP).. not sure what the difference is between GYP/GN compiling rules -- linked it to your bug per the size_t->DWORD issue which has been common with clang but it's probably not related if the difference between GN/GYP has nothing to do with clang...
Message was sent while issue was closed.
unless you explicitly opted in to it, you're not using clang. probably just the gn build being busted then.
Message was sent while issue was closed.
On 2016/02/03 18:44:46, Nico wrote: > unless you explicitly opted in to it, you're not using clang. Ah okay, thanks. > probably just the gn build being busted then. Well, it's a valid warning, weird that GN build gives it but not GYP (maybe more warnings are turned into errors by default in GN?)
Message was sent while issue was closed.
gn defaults to chromium_code, gyp doesn't – but https://codereview.chromium.org/1580673004/ flipped that for this target not too long ago i thought On Wed, Feb 3, 2016 at 1:46 PM, <gab@chromium.org> wrote: > On 2016/02/03 18:44:46, Nico wrote: > > unless you explicitly opted in to it, you're not using clang. > > Ah okay, thanks. > > > probably just the gn build being busted then. > > Well, it's a valid warning, weird that GN build gives it but not GYP (maybe more > warnings are turned into errors by default in GN?) > https://codereview.chromium.org/1662683002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Ah I see, so then somewhat related to that bug :-) On Wed, Feb 3, 2016 at 1:49 PM Nico Weber <thakis@chromium.org> wrote: > gn defaults to chromium_code, gyp doesn't – but > https://codereview.chromium.org/1580673004/ flipped that for this target > not too long ago i thought > > On Wed, Feb 3, 2016 at 1:46 PM, <gab@chromium.org> wrote: > >> On 2016/02/03 18:44:46, Nico wrote: >> > unless you explicitly opted in to it, you're not using clang. >> >> Ah okay, thanks. >> >> > probably just the gn build being busted then. >> >> Well, it's a valid warning, weird that GN build gives it but not GYP (maybe more >> warnings are turned into errors by default in GN?) >> https://codereview.chromium.org/1662683002/ >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
