|
|
Created:
4 years, 2 months ago by nisse-webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete always-zero ByteBufferWriter::start_.
Likely a left-over since cl https://codereview.webrtc.org/1821083002
BUG=None
Committed: https://crrev.com/d3c40089fa5b03de918e0660f79824dd65e4d5e8
Cr-Commit-Position: refs/heads/master@{#14732}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Delete useless memmove. #
Messages
Total messages: 23 (7 generated)
nisse@webrtc.org changed reviewers: + jbauch@webrtc.org, kwiberg@webrtc.org, tommi@webrtc.org
Other refactorings I'm considering: Deleting the conditional byte-order logic, since this is currently used exclusively for network byte-order. Change to use uint8_t* rather than char* (something similar done to the Buffer class in cl https://webrtc-codereview.appspot.com/42989004).
tommi@chromium.org changed reviewers: + tommi@chromium.org
lgtm
lgtm
lgtm Because it's not enough to have a change of this magnitude reviewed by only two engineers. :-)
The CQ bit was checked by nisse@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/10/21 11:56:52, kwiberg-webrtc wrote: > lgtm > > Because it's not enough to have a change of this magnitude reviewed by only two > engineers. :-) I was also hoping for some advice on further changes...
https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc File webrtc/base/bytebuffer.cc (right): https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc#n... webrtc/base/bytebuffer.cc:124: memmove(bytes_, bytes_, len); Sorry, didn't notice this in the first place, but the memmove is a no-op now and can be removed.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
On 2016/10/21 09:55:30, nisse-webrtc wrote: > Other refactorings I'm considering: > > Deleting the conditional byte-order logic, since this is currently used > exclusively for network byte-order. Probably makes sense. > Change to use uint8_t* rather than char* (something similar done to the Buffer > class in cl https://webrtc-codereview.appspot.com/42989004). Would it make sense for this class to use a Buffer internally?
https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc File webrtc/base/bytebuffer.cc (right): https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc#n... webrtc/base/bytebuffer.cc:124: memmove(bytes_, bytes_, len); On 2016/10/21 13:11:19, joachim wrote: > Sorry, didn't notice this in the first place, but the memmove is a no-op now and > can be removed. So it DOES take three people to review this... :-)
https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc File webrtc/base/bytebuffer.cc (right): https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc#n... webrtc/base/bytebuffer.cc:124: memmove(bytes_, bytes_, len); On 2016/10/21 15:08:42, kwiberg-webrtc wrote: > On 2016/10/21 13:11:19, joachim wrote: > > Sorry, didn't notice this in the first place, but the memmove is a no-op now > and > > can be removed. > > So it DOES take three people to review this... :-) Indeed!
On 2016/10/21 15:08:33, kwiberg-webrtc wrote: > Would it make sense for this class to use a Buffer internally? Sounds reasonable, but I haven't looked at the details...
https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc File webrtc/base/bytebuffer.cc (right): https://codereview.chromium.org/2440083002/diff/1/webrtc/base/bytebuffer.cc#n... webrtc/base/bytebuffer.cc:124: memmove(bytes_, bytes_, len); On 2016/10/21 13:11:19, joachim wrote: > Sorry, didn't notice this in the first place, but the memmove is a no-op now and > can be removed. Done.
The CQ bit was checked by nisse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbauch@webrtc.org, kwiberg@webrtc.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2440083002/#ps20001 (title: "Delete useless memmove.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Delete always-zero ByteBufferWriter::start_. Likely a left-over since cl https://codereview.webrtc.org/1821083002 BUG=None ========== to ========== Delete always-zero ByteBufferWriter::start_. Likely a left-over since cl https://codereview.webrtc.org/1821083002 BUG=None Committed: https://crrev.com/d3c40089fa5b03de918e0660f79824dd65e4d5e8 Cr-Commit-Position: refs/heads/master@{#14732} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d3c40089fa5b03de918e0660f79824dd65e4d5e8 Cr-Commit-Position: refs/heads/master@{#14732} |