Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by kwiberg-webrtc Modified:
4 years, 7 months ago Reviewers:
tommi, pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Descriptionrtc::Optional<T>: Don't secretly contain a default-constructed T when empty
Instead, use a neat trick with union to ensure that we have a T only
when we're supposed to (and just a bunch of unused memory otherwise).
This is how std::optional behaves, so it makes sense for us to do the
same (and it's convenient, too, since we don't have to pay for the
default-constructed T, and we support types that don't have default
constructors).
Doing this became possible recently when we dropped support for MSVC
2013, which didn't support unions containing non-trivial types.
Committed: https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a
Cr-Commit-Position: refs/heads/master@{#12664}
Patch Set 1 : #Patch Set 2 : rebase #
Created: 4 years, 7 months ago
Messages
Total messages: 19 (11 generated)
Description was changed from ========== Try union with class member ========== to ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. ========== to ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. ==========
kwiberg@webrtc.org changed reviewers: + pthatcher@webrtc.org
I've had this lying around since before Optional was initially landed, but it didn't work with MSVC 2013. And then, earlier tonight, I needed to use Optional<T> for a T that didn't have a default constructor, so rather than just bolting on an ugly default constructor, I thought I'd brush the dust off of this CL and see if Microsoft had caught up. The trybots say they have. To get a feel for what sort of effect this has, take a look at the diff for the unit tests.
kwiberg@webrtc.org changed reviewers: + tommi@webrtc.org
+tommi@ Also, bringing up the question of potentially dropping our Optional implementation (and therefore this CL) and stealing Chromium's instead (since they have one now), to reduce the amount of wheel reinvention.
lgtm. that' pretty neat.
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896833004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896833004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7300) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7286) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9624) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/9548) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14829) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13482) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/14535) linux_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/10711) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1871) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/14453) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11179) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2974) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9195) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14933) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14527)
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/1896833004/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896833004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896833004/40001
Message was sent while issue was closed.
Description was changed from ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. ========== to ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. ========== to ========== rtc::Optional<T>: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. Committed: https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a Cr-Commit-Position: refs/heads/master@{#12664} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a Cr-Commit-Position: refs/heads/master@{#12664} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
