Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3002283002:
Implement move constructor for RTPFragmentationHeader. (Closed)
|
Created:
3 years, 3 months ago by sakal Modified:
3 years, 3 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement move constructor for RTPFragmentationHeader.
This allows returning RTPFragmentationHeader from a method and assigning
the results to a variable.
BUG=webrtc:7760
Review-Url: https://codereview.webrtc.org/3002283002
Cr-Commit-Position: refs/heads/master@{#19556}
Committed: https://chromium.googlesource.com/external/webrtc/+/34960de3fb4257a2bba9847e8df3ef76dee59e77
Patch Set 1 : Use assign for int. #
Total comments: 2
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Address comments 2. #Messages
Total messages: 26 (18 generated)
Patchset #1 (id:1) has been deleted
sakal@webrtc.org changed reviewers: + kwiberg@webrtc.org
PTAL
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but see insidious suggestion https://codereview.webrtc.org/3002283002/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/3002283002/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:101: } You could turn lines 96-100 into a swap() implementation, and then define move construction and move assignment in terms of that. (It's usually not a bad idea to support the full set of operations if it doesn't cost you anything.)
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
https://codereview.webrtc.org/3002283002/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/3002283002/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:101: } On 2017/08/27 22:59:24, kwiberg-webrtc wrote: > You could turn lines 96-100 into a swap() implementation, and then define move > construction and move assignment in terms of that. (It's usually not a bad idea > to support the full set of operations if it doesn't cost you anything.) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
https://codereview.webrtc.org/3002283002/diff/40001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/3002283002/diff/40001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:233: } To make this useful with the standard swapping idiom (see http://en.cppreference.com/w/cpp/concept/Swappable)---and to use this idiom for the member swap calls---define swap() like this in the public section of the class: friend void swap(RTPFragmentationHeader&a, RTPFragmentationHeader&b) { using std::swap; swap(a.fragmentationVectorSize, b.fragmentationVectorSize); // ... and the rest of the members ... }
https://codereview.webrtc.org/3002283002/diff/40001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/3002283002/diff/40001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:233: } On 2017/08/28 08:36:20, kwiberg-webrtc wrote: > To make this useful with the standard swapping idiom (see > http://en.cppreference.com/w/cpp/concept/Swappable)---and to use this idiom for > the member swap calls---define swap() like this in the public section of the > class: > > friend void swap(RTPFragmentationHeader&a, RTPFragmentationHeader&b) { > using std::swap; > swap(a.fragmentationVectorSize, b.fragmentationVectorSize); > // ... and the rest of the members ... > } Done.
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
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 sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/3002283002/#ps60001 (title: "Address comments 2.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1503915632378600,
"parent_rev": "cfc29f4c9ad0740a33461f5acd672ccfaf25d262", "commit_rev":
"34960de3fb4257a2bba9847e8df3ef76dee59e77"}
Message was sent while issue was closed.
Description was changed from ========== Implement move constructor for RTPFragmentationHeader. This allows returning RTPFragmentationHeader from a method and assigning the results to a variable. BUG=webrtc:7760 ========== to ========== Implement move constructor for RTPFragmentationHeader. This allows returning RTPFragmentationHeader from a method and assigning the results to a variable. BUG=webrtc:7760 Review-Url: https://codereview.webrtc.org/3002283002 Cr-Commit-Position: refs/heads/master@{#19556} Committed: https://chromium.googlesource.com/external/webrtc/+/34960de3fb4257a2bba9847e8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/34960de3fb4257a2bba9847e8...
Message was sent while issue was closed.
Excellent! lgtm (again) |
