Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738063005:
Use native (optimized) functions for byte order conversion. (Closed)
DescriptionUse native (optimized) functions for byte order conversion.
Instead of manually copying single bytes, the native functions like "htobe32"
are used.
BUG=None
Review-Url: https://codereview.webrtc.org/2738063005
Cr-Commit-Position: refs/heads/master@{#17277}
Committed: https://chromium.googlesource.com/external/webrtc/+/38fd44e51cbde57a277b85368ad247077642d13d
Patch Set 1 #Patch Set 2 : Fix compiling on Mac. #Patch Set 3 : Fix compilation on Windows. #
Total comments: 4
Messages
Total messages: 13 (5 generated)
jbauch@webrtc.org changed reviewers: + tommi@webrtc.org
Ptal, this implements an old TODO. https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h File webrtc/base/byteorder.h (right): https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... webrtc/base/byteorder.h:18: #include "webrtc/base/basictypes.h" Moved here so the "RTC_ARCH_CPU_" defines are available below. https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... webrtc/base/byteorder.h:49: #if defined(RTC_ARCH_CPU_LITTLE_ENDIAN) To simplify things we could assume Windows is always little endian (which should almost always be the case). This code explicitly supports both.
On 2017/03/09 23:36:53, joachim wrote: > Ptal, this implements an old TODO. > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h > File webrtc/base/byteorder.h (right): > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... > webrtc/base/byteorder.h:18: #include "webrtc/base/basictypes.h" > Moved here so the "RTC_ARCH_CPU_" defines are available below. > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... > webrtc/base/byteorder.h:49: #if defined(RTC_ARCH_CPU_LITTLE_ENDIAN) > To simplify things we could assume Windows is always little endian (which should > almost always be the case). This code explicitly supports both. Ping, or should I add somebody else for review if you are busy?
lgtm https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h File webrtc/base/byteorder.h (right): https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... webrtc/base/byteorder.h:18: #include "webrtc/base/basictypes.h" On 2017/03/09 23:36:53, joachim wrote: > Moved here so the "RTC_ARCH_CPU_" defines are available below. Acknowledged. https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... webrtc/base/byteorder.h:49: #if defined(RTC_ARCH_CPU_LITTLE_ENDIAN) On 2017/03/09 23:36:53, joachim wrote: > To simplify things we could assume Windows is always little endian (which should > almost always be the case). This code explicitly supports both. Acknowledged.
On 2017/03/16 16:37:02, tommi (webrtc) wrote: > lgtm > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h > File webrtc/base/byteorder.h (right): > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... > webrtc/base/byteorder.h:18: #include "webrtc/base/basictypes.h" > On 2017/03/09 23:36:53, joachim wrote: > > Moved here so the "RTC_ARCH_CPU_" defines are available below. > > Acknowledged. > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... > webrtc/base/byteorder.h:49: #if defined(RTC_ARCH_CPU_LITTLE_ENDIAN) > On 2017/03/09 23:36:53, joachim wrote: > > To simplify things we could assume Windows is always little endian (which > should > > almost always be the case). This code explicitly supports both. > > Acknowledged. oh and sorry for the delay, I had somehow missed the review request
On 2017/03/16 16:37:43, tommi (webrtc) wrote: > On 2017/03/16 16:37:02, tommi (webrtc) wrote: > > lgtm > > > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h > > File webrtc/base/byteorder.h (right): > > > > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... > > webrtc/base/byteorder.h:18: #include "webrtc/base/basictypes.h" > > On 2017/03/09 23:36:53, joachim wrote: > > > Moved here so the "RTC_ARCH_CPU_" defines are available below. > > > > Acknowledged. > > > > > https://codereview.webrtc.org/2738063005/diff/40001/webrtc/base/byteorder.h#n... > > webrtc/base/byteorder.h:49: #if defined(RTC_ARCH_CPU_LITTLE_ENDIAN) > > On 2017/03/09 23:36:53, joachim wrote: > > > To simplify things we could assume Windows is always little endian (which > > should > > > almost always be the case). This code explicitly supports both. > > > > Acknowledged. > > oh and sorry for the delay, I had somehow missed the review request No problem!
The CQ bit was checked by jbauch@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Use native (optimized) functions for byte order conversion. Instead of manually copying single bytes, the native functions like "htobe32" are used. Bug=None ========== to ========== Use native (optimized) functions for byte order conversion. Instead of manually copying single bytes, the native functions like "htobe32" are used. BUG=None ==========
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489682830546000,
"parent_rev": "2a420ce8c4dcb2c277707585e00d56ca306ed38c", "commit_rev":
"38fd44e51cbde57a277b85368ad247077642d13d"}
Message was sent while issue was closed.
Description was changed from ========== Use native (optimized) functions for byte order conversion. Instead of manually copying single bytes, the native functions like "htobe32" are used. BUG=None ========== to ========== Use native (optimized) functions for byte order conversion. Instead of manually copying single bytes, the native functions like "htobe32" are used. BUG=None Review-Url: https://codereview.webrtc.org/2738063005 Cr-Commit-Position: refs/heads/master@{#17277} Committed: https://chromium.googlesource.com/external/webrtc/+/38fd44e51cbde57a277b85368... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/38fd44e51cbde57a277b85368...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2757703002/ by jbauch@webrtc.org. The reason for reverting is: Breaks Chromium FYI bots: http://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/builds... http://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/buil... FAILED: newlib_pnacl/obj/third_party/webrtc/base/rtc_base/networkmonitor.o /b/c/goma_client/gomacc ../../native_client/toolchain/linux_x86/pnacl_newlib/bin/pnacl-clang++ -MMD -MF newlib_pnacl/obj/third_party/webrtc/base/rtc_base/networkmonitor.o.d -DNACL_TC_REV=62bfd122aee87d4eb4a7876950e18c793c626cd0 -Dtimezone=_timezone -DV8_DEPRECATION_WARNINGS -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND -DWEBRTC_RESTRICT_LOGGING -DEXPAT_RELATIVE_PATH -DHAVE_SCTP -DENABLE_EXTERNAL_AUTH -DHAVE_WEBRTC_VIDEO -DHAVE_WEBRTC_VOICE -DLOGGING_INSIDE_WEBRTC -DUSE_WEBRTC_DEV_BRANCH -DFEATURE_ENABLE_VOICEMAIL -DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH -DNO_MAIN_THREAD_WRAPPING -DNO_SOUND_SYSTEM -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_POSIX -DNO_MAIN_THREAD_WRAPPING -I../../third_party/boringssl/src/include -I../.. -Inewlib_pnacl/gen -I../../third_party/webrtc_overrides -I../../third_party -I../../third_party/boringssl/src/include -I../../native_client_sdk/src/libraries -I../../native_client_sdk/src/libraries/nacl_io/include -I../../native_client_sdk/src/libraries/third_party/newlib-extras -Wno-uninitialized -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -fcolor-diagnostics -O2 -fno-ident -fdata-sections -ffunction-sections -g0 -fvisibility=hidden -Werror -Wno-unused-variable -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -fvisibility-inlines-hidden -std=gnu++11 -fno-rtti -fno-exceptions -c ../../third_party/webrtc/base/networkmonitor.cc -o newlib_pnacl/obj/third_party/webrtc/base/rtc_base/networkmonitor.o In file included from ../../third_party/webrtc/base/networkmonitor.cc:11: In file included from ../../third_party/webrtc/base/networkmonitor.h:16: In file included from ../../third_party/webrtc/base/thread.h:25: In file included from ../../third_party/webrtc/base/messagequeue.h:31: In file included from ../../third_party/webrtc/base/socketserver.h:15: In file included from ../../third_party/webrtc/base/socketfactory.h:14: In file included from ../../third_party/webrtc/base/socket.h:30: In file included from ../../third_party/webrtc/base/socketaddress.h:18: In file included from ../../third_party/webrtc/base/ipaddress.h:29: ../../third_party/webrtc/base/byteorder.h:37:10: fatal error: 'endian.h' file not found #include <endian.h> ^ 1 error generated.. |
