Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1763983002:
Change scoped_ptr to a type alias for std::unique_ptr on OS_WIN (Closed)
|
Created:
4 years, 9 months ago by dcheng Modified:
4 years, 9 months ago CC:
Anand Mistry (off Chromium), chromium-reviews, chromoting-reviews_chromium.org, gavinp+memory_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange scoped_ptr to a type alias for std::unique_ptr on OS_WIN
This also removes the infinite recursion part of the ReferenceCycle test.
While it's nice not to crash like this, [unique.ptr.single.dtor] doesn't
actually require ~unique_ptr to reset the stored pointer to nullptr on
destruction. Thus, infinite recursion, while not useful, is allowed per
the spec.
BUG=554298, 579269
Committed: https://crrev.com/9fd4ab64bd402859ccea0c5213b0a478f9d6ae5a
Cr-Commit-Position: refs/heads/master@{#379962}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Delete problematic test. #Patch Set 3 : git add -p fail #Patch Set 4 : Fix bluetooth #
Messages
Total messages: 36 (16 generated)
dcheng@chromium.org changed reviewers: + danakj@chromium.org, joedow@chromium.org, vitalybuka@chromium.org
+danakj for //base +vitalybuka for //printing +joedow for //remoting
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763983002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763983002/1
On 2016/03/04 at 02:17:32, commit-bot wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1763983002/1 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1763983002/1 Hmm… so this is fun. ScopedPtrTest.ReferenceCycle crashes on Windows if scoped_ptr is an alias. I'll take a closer look at what's going on tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... File printing/backend/printing_info_win.h (right): https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... printing/backend/printing_info_win.h:29: return buffer_ != nullptr; is any warning without this? I thought Chromium prefer checking pointers without comparing with null.
https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... File printing/backend/printing_info_win.h (right): https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... printing/backend/printing_info_win.h:29: return buffer_ != nullptr; On 2016/03/04 at 18:25:17, Vitaly Buka wrote: > is any warning without this? > I thought Chromium prefer checking pointers without comparing with null. It depends on the reviewer =P With C++11 and explicit operator bool, this can no longer implicitly convert, since it's not a place where a "context conversion to bool" is allowed (an example of somewhere where it is allowed is if (x) { ... }). So we have several choices: - return static_cast<bool>(buffer_); - return !!buffer_; (my personal preferred option) - return buffer_ != nullptr; (some reviewers expressed that this might be clearer)
https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... File printing/backend/printing_info_win.h (right): https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... printing/backend/printing_info_win.h:29: return buffer_ != nullptr; Thanks. I like !! too, but current one is lgtm too.
lgtm for remoting. https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... File printing/backend/printing_info_win.h (right): https://codereview.chromium.org/1763983002/diff/1/printing/backend/printing_i... printing/backend/printing_info_win.h:29: return buffer_ != nullptr; On 2016/03/04 18:34:39, Vitaly Buka wrote: > Thanks. I like !! too, but current one is lgtm too. I like !! as well to boolify but I have seen feedback both ways.
So it turns out the ReferenceCycle test that we don't infinitely recurse in the destructor [1] is crashing, since it infinitely recurses with VC++2013's unique_ptr. VC++2013 does not reset the stored pointer value on ~unique_ptr destruction, so the stack ends up exploding. However, the C++11 spec [unique.ptr.single.dtor] does not require that the unique_ptr destructor reset the stored pointer value to nullptr in the destructor… so technically, this is perfectly legal behavior per the spec. C++11 owners, maybe we can just remove the infinite recursion part of that test? [1] Added in https://codereview.chromium.org/1356483002
On 2016/03/04 23:52:21, dcheng wrote: > So it turns out the ReferenceCycle test that we don't infinitely recurse in the > destructor [1] is crashing, since it infinitely recurses with VC++2013's > unique_ptr. VC++2013 does not reset the stored pointer value on ~unique_ptr > destruction, so the stack ends up exploding. > > However, the C++11 spec [unique.ptr.single.dtor] does not require that the > unique_ptr destructor reset the stored pointer value to nullptr in the > destructor… so technically, this is perfectly legal behavior per the spec. > > C++11 owners, maybe we can just remove the infinite recursion part of that test? Yah, okay. We're not going to be testing unique_ptr anyways. It is nicer if the implementation of unique_ptr gives a nicer crash stack, and we'll get that where we use libcxx. I don't think this is a blocker from using unique_ptr. LGTM > [1] Added in https://codereview.chromium.org/1356483002
Description was changed from ========== Change scoped_ptr to a type alias for std::unique_ptr on OS_WIN BUG=554298,579269 ========== to ========== Change scoped_ptr to a type alias for std::unique_ptr on OS_WIN This also removes the infinite recursion part of the ReferenceCycle test. While it's nice not to crash like this, [unique.ptr.single.dtor] doesn't actually require ~unique_ptr to reset the stored pointer to nullptr on destruction. Thus, infinite recursion, while not useful, is allowed per the spec. BUG=554298,579269 ==========
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, joedow@chromium.org, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/1763983002/#ps20001 (title: "Delete problematic test.")
The CQ bit was unchecked by dcheng@chromium.org
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, joedow@chromium.org, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/1763983002/#ps40001 (title: "git add -p fail")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763983002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
+keybuk, mind taking a look? This fixes a test failure in BluetoothGattServiceTest.GetCharacteristic_CharacteristicRemoved when using unique_ptr: the MSVC unique_ptr does not reset the stored pointer to null when it's destroyed. The failure happens because: 1) The characteristic calls GattCharacteristicRemoved() from the destructor. 2) TestBluetoothAdapterObserver does some sanity checks. 3) One of these is that the removed characteristic can no longer be looked up in the service. 4) ~unique_ptr doesn't set the value to null before calling the destructor. 5) Thus, EXPECT_FALSE(characteristic->GetService()->last_gatt_characteristic_id_)) (line 238 of test_bluetooth_adapter_observer.cc) ends up failing.
dcheng@chromium.org changed reviewers: + scheib@chromium.org
Also +scheib, who reviewed the patch (https://codereview.chromium.org/1690133002) that added this test.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763983002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763983002/60001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, joedow@chromium.org, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/1763983002/#ps60001 (title: "Fix bluetooth")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763983002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763983002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change scoped_ptr to a type alias for std::unique_ptr on OS_WIN This also removes the infinite recursion part of the ReferenceCycle test. While it's nice not to crash like this, [unique.ptr.single.dtor] doesn't actually require ~unique_ptr to reset the stored pointer to nullptr on destruction. Thus, infinite recursion, while not useful, is allowed per the spec. BUG=554298,579269 ========== to ========== Change scoped_ptr to a type alias for std::unique_ptr on OS_WIN This also removes the infinite recursion part of the ReferenceCycle test. While it's nice not to crash like this, [unique.ptr.single.dtor] doesn't actually require ~unique_ptr to reset the stored pointer to nullptr on destruction. Thus, infinite recursion, while not useful, is allowed per the spec. BUG=554298,579269 Committed: https://crrev.com/9fd4ab64bd402859ccea0c5213b0a478f9d6ae5a Cr-Commit-Position: refs/heads/master@{#379962} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9fd4ab64bd402859ccea0c5213b0a478f9d6ae5a Cr-Commit-Position: refs/heads/master@{#379962} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
