Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522483002:
Remove thread-id wraparounds in event tracing. (Closed)
|
Created:
5 years ago by pbos-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove thread-id wraparounds in event tracing.
Prints tid as a DWORD on Windows and an int on other systems.
BUG=
R=thakis@chromium.org, tommi@webrtc.org
Committed: https://crrev.com/43e4e23eebdc3c67269c1962d279a61bdba2c2b6
Cr-Commit-Position: refs/heads/master@{#10992}
Patch Set 1 #
Total comments: 2
Patch Set 2 : %lu #Messages
Total messages: 11 (2 generated)
PTAL, I think this should be fine on clang-win as well and I'd prefer to not do the casting.
lgtm
https://codereview.webrtc.org/1522483002/diff/1/webrtc/base/event_tracer.cc File webrtc/base/event_tracer.cc (right): https://codereview.webrtc.org/1522483002/diff/1/webrtc/base/event_tracer.cc#n... webrtc/base/event_tracer.cc:123: ", \"tid\": %" PRIu32 This fails like so: [1/2] CXX obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj FAILED: ninja -t msvc -e environment.x64 -- ../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showI ncludes /FC @obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj.rsp /c ../../third_party/webrtc/base/event_t racer.cc /Foobj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj /Fdobj/third_party/webrtc/base/rtc_base_appro ved_cc.pdb ../../third_party/webrtc/base/event_tracer.cc(129,46) : error: format specifies type 'unsigned int' but the argument ha s type 'rtc::PlatformThreadId' (aka 'unsigned long') [-Werror,-Wformat] e.phase, e.timestamp, e.pid, e.tid); ^~~~~ 1 error generated. ninja: build stopped: subcommand failed. This is probably because of bit in clang's inttypes.h: #if defined(_MSC_VER) && _MSC_VER < 1900 /* MSVC headers define int32_t as int, but PRIx32 as "lx" instead of "x". * This triggers format warnings, so fix it up here. */ #define PRIu32 "u" Why go through this trouble though? PRIu32 is for printing an uint32_t, not an rtc::PlatformThreadId aka DWORD. But since you know it's Windows, you can probably just use "%ld", as dword is always an unsigned long (since long is 32-bit on windows in both 32-bit and 64-bit) I think.
On 2015/12/11 18:18:44, Nico wrote: > https://codereview.webrtc.org/1522483002/diff/1/webrtc/base/event_tracer.cc > File webrtc/base/event_tracer.cc (right): > > https://codereview.webrtc.org/1522483002/diff/1/webrtc/base/event_tracer.cc#n... > webrtc/base/event_tracer.cc:123: ", \"tid\": %" PRIu32 > This fails like so: > > [1/2] CXX obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj > FAILED: ninja -t msvc -e environment.x64 -- > ../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showI > ncludes /FC @obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj.rsp > /c ../../third_party/webrtc/base/event_t > racer.cc /Foobj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj > /Fdobj/third_party/webrtc/base/rtc_base_appro > ved_cc.pdb > ../../third_party/webrtc/base/event_tracer.cc(129,46) : error: format specifies > type 'unsigned int' but the argument ha > s type 'rtc::PlatformThreadId' (aka 'unsigned long') [-Werror,-Wformat] > e.phase, e.timestamp, e.pid, e.tid); > ^~~~~ > 1 error generated. > ninja: build stopped: subcommand failed. > > > > This is probably because of bit in clang's inttypes.h: > > #if defined(_MSC_VER) && _MSC_VER < 1900 > /* MSVC headers define int32_t as int, but PRIx32 as "lx" instead of "x". > * This triggers format warnings, so fix it up here. */ > > #define PRIu32 "u" > > > Why go through this trouble though? PRIu32 is for printing an uint32_t, not an > rtc::PlatformThreadId aka DWORD. But since you know it's Windows, you can > probably just use "%ld", as dword is always an unsigned long (since long is > 32-bit on windows in both 32-bit and 64-bit) I think. Agree with Nico.
%lu
PTAL, if this compiles on all our Windows bots I'm cool with it https://codereview.webrtc.org/1522483002/diff/1/webrtc/base/event_tracer.cc File webrtc/base/event_tracer.cc (right): https://codereview.webrtc.org/1522483002/diff/1/webrtc/base/event_tracer.cc#n... webrtc/base/event_tracer.cc:123: ", \"tid\": %" PRIu32 On 2015/12/11 18:18:44, Nico wrote: > This fails like so: > > [1/2] CXX obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj > FAILED: ninja -t msvc -e environment.x64 -- > ../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showI > ncludes /FC @obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj.rsp > /c ../../third_party/webrtc/base/event_t > racer.cc /Foobj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj > /Fdobj/third_party/webrtc/base/rtc_base_appro > ved_cc.pdb > ../../third_party/webrtc/base/event_tracer.cc(129,46) : error: format specifies > type 'unsigned int' but the argument ha > s type 'rtc::PlatformThreadId' (aka 'unsigned long') [-Werror,-Wformat] > e.phase, e.timestamp, e.pid, e.tid); > ^~~~~ > 1 error generated. > ninja: build stopped: subcommand failed. > > > > This is probably because of bit in clang's inttypes.h: > > #if defined(_MSC_VER) && _MSC_VER < 1900 > /* MSVC headers define int32_t as int, but PRIx32 as "lx" instead of "x". > * This triggers format warnings, so fix it up here. */ > > #define PRIu32 "u" > > > Why go through this trouble though? PRIu32 is for printing an uint32_t, not an > rtc::PlatformThreadId aka DWORD. But since you know it's Windows, you can > probably just use "%ld", as dword is always an unsigned long (since long is > 32-bit on windows in both 32-bit and 64-bit) I think. assuming %lu, done.
lgtm, that builds with clang-cl as best I can tell.
Description was changed from ========== Remove thread-id wraparounds in event tracing. Prints tid as a DWORD on Windows and an int on other systems. BUG= R=tommi@webrtc.org, thakis@chromium.org ========== to ========== Remove thread-id wraparounds in event tracing. Prints tid as a DWORD on Windows and an int on other systems. BUG= R=thakis@chromium.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/43e4e23eebdc3c67269c1962d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 43e4e23eebdc3c67269c1962d279a61bdba2c2b6 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Remove thread-id wraparounds in event tracing. Prints tid as a DWORD on Windows and an int on other systems. BUG= R=thakis@chromium.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/43e4e23eebdc3c67269c1962d... ========== to ========== Remove thread-id wraparounds in event tracing. Prints tid as a DWORD on Windows and an int on other systems. BUG= R=thakis@chromium.org, tommi@webrtc.org Committed: https://crrev.com/43e4e23eebdc3c67269c1962d279a61bdba2c2b6 Cr-Commit-Position: refs/heads/master@{#10992} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/43e4e23eebdc3c67269c1962d279a61bdba2c2b6 Cr-Commit-Position: refs/heads/master@{#10992} |
