Enable tracing on rtcstats_integrationtest.cc

Enable tracing on rtcstats_integrationtest.cc

BUG=chromium:653087
Review-Url: https://codereview.webrtc.org/2979203002
Cr-Commit-Position: refs/heads/master@{#19076}
Committed: https://chromium.googlesource.com/external/webrtc/+/80c829f25382f98e97b30a5803322552f350f983

[ehmaldonado_webrtc, 2017-07-18 12:15:53]

ehmaldonado@webrtc.org changed reviewers:
+ hbos@webrtc.org, tommi@webrtc.org

[tommi, 2017-07-18 12:22:54]

nice. lgtm with the below suggestions incorporated.

https://codereview.webrtc.org/2979203002/diff/20001/webrtc/pc/rtcstats_integr...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2979203002/diff/20001/webrtc/pc/rtcstats_integr...
webrtc/pc/rtcstats_integrationtest.cc:35: reinterpret_cast<const unsigned
char*>("webrtc_stats");
static_cast, however the cast shouldn't be here.  instead do:

const char kWebRTCStatsCategory[] = "webrtc_stats";

and change the function (which doesn't need to be static since it's in an
anonymous namespace):

const unsigned char* GetCategoryEnabledHandler(const char* name) {
  return static_cast<const unsigned char*>(&kWebRTCStatsCategory[0]);
}

You could also declare the constant as a static inside the scope of the
function.

[ehmaldonado_webrtc, 2017-07-18 12:36:19]

PTAL

https://codereview.webrtc.org/2979203002/diff/20001/webrtc/pc/rtcstats_integr...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2979203002/diff/20001/webrtc/pc/rtcstats_integr...
webrtc/pc/rtcstats_integrationtest.cc:35: reinterpret_cast<const unsigned
char*>("webrtc_stats");
On 2017/07/18 12:22:54, tommi (wëbrtc) wrote:
> static_cast, however the cast shouldn't be here.  instead do:
> 
> const char kWebRTCStatsCategory[] = "webrtc_stats";
> 
> and change the function (which doesn't need to be static since it's in an
> anonymous namespace):
> 
> const unsigned char* GetCategoryEnabledHandler(const char* name) {
>   return static_cast<const unsigned char*>(&kWebRTCStatsCategory[0]);
> }
> 
> You could also declare the constant as a static inside the scope of the
> function.

I get:

error: static_cast from 'const char *' to 'const unsigned char *' is not allowed

[tommi, 2017-07-18 12:45:04]

On 2017/07/18 12:36:19, ehmaldonado_webrtc wrote:
> PTAL
> 
>
https://codereview.webrtc.org/2979203002/diff/20001/webrtc/pc/rtcstats_integr...
> File webrtc/pc/rtcstats_integrationtest.cc (right):
> 
>
https://codereview.webrtc.org/2979203002/diff/20001/webrtc/pc/rtcstats_integr...
> webrtc/pc/rtcstats_integrationtest.cc:35: reinterpret_cast<const unsigned
> char*>("webrtc_stats");
> On 2017/07/18 12:22:54, tommi (wëbrtc) wrote:
> > static_cast, however the cast shouldn't be here.  instead do:
> > 
> > const char kWebRTCStatsCategory[] = "webrtc_stats";
> > 
> > and change the function (which doesn't need to be static since it's in an
> > anonymous namespace):
> > 
> > const unsigned char* GetCategoryEnabledHandler(const char* name) {
> >   return static_cast<const unsigned char*>(&kWebRTCStatsCategory[0]);
> > }
> > 
> > You could also declare the constant as a static inside the scope of the
> > function.
> 
> I get:
> 
> error: static_cast from 'const char *' to 'const unsigned char *' is not
allowed

hmm.. strange... OK, reintperpret_cast it is then. Also, you could shorten the
whole thing:

 const unsigned char* GetCategoryEnabledHandler(const char* name) {
  return reinterpret_cast<const unsigned char*>("webrtc_stats");
}

[ehmaldonado_webrtc, 2017-07-18 13:11:06]

On 2017/07/18 12:45:04, tommi (wëbrtc) wrote:
> Also, you could shorten the whole thing:
> 
>  const unsigned char* GetCategoryEnabledHandler(const char* name) {
>   return reinterpret_cast<const unsigned char*>("webrtc_stats");
> }

Done.
Why does this work? Where is the memory allocated and what is the lifetime?

[hbos, 2017-07-18 13:48:47]

On 2017/07/18 13:11:06, ehmaldonado_webrtc wrote:
> On 2017/07/18 12:45:04, tommi (wëbrtc) wrote:
> > Also, you could shorten the whole thing:
> > 
> >  const unsigned char* GetCategoryEnabledHandler(const char* name) {
> >   return reinterpret_cast<const unsigned char*>("webrtc_stats");
> > }
> 
> Done.
> Why does this work? Where is the memory allocated and what is the lifetime?

"string literals" exists for the lifetime of the program, you get it as a const
char*. They'll exist somewhere in the data section, the compiler handles it for
you. If you do this:
const char* str1 = "hello";
const char* str2 = "hello";
The compiler will probably assign them the same value (address to char array) as
long as they're in they are in the same binary. Linking different modules
together though str1 == str2 might not always hold even if they're both "hello".

If you do const char str[] instead you get a new array, with whatever lifetime
str has depending on where you declare it. Just like any other array or
variable. In the pointer case you have a lifetime of the pointer variable
(depending on where declared) and a lifetime of the memory pointed to (lifetime
of program).

You have to do the reinterpret_cast because the signed-ness of char is up to the
compiler/platform, the data you're pointing to may be a signed char[] or it may
be an unsigned char[]. It is safe to reinterpret cast because what you're
pointing to is guaranteed to have the lifetime of the program and the elements
of the array have the correct size_of. The signed/unsigned just affects what
value you interpret these bytes as having.

[ehmaldonado_webrtc, 2017-07-18 13:55:15]

On 2017/07/18 13:48:47, hbos wrote:
> On 2017/07/18 13:11:06, ehmaldonado_webrtc wrote:
> > On 2017/07/18 12:45:04, tommi (wëbrtc) wrote:
> > > Also, you could shorten the whole thing:
> > > 
> > >  const unsigned char* GetCategoryEnabledHandler(const char* name) {
> > >   return reinterpret_cast<const unsigned char*>("webrtc_stats");
> > > }
> > 
> > Done.
> > Why does this work? Where is the memory allocated and what is the lifetime?
> 
> "string literals" exists for the lifetime of the program, you get it as a
const
> char*. They'll exist somewhere in the data section, the compiler handles it
for
> you. If you do this:
> const char* str1 = "hello";
> const char* str2 = "hello";
> The compiler will probably assign them the same value (address to char array)
as
> long as they're in they are in the same binary. Linking different modules
> together though str1 == str2 might not always hold even if they're both
"hello".
> 
> If you do const char str[] instead you get a new array, with whatever lifetime
> str has depending on where you declare it. Just like any other array or
> variable. In the pointer case you have a lifetime of the pointer variable
> (depending on where declared) and a lifetime of the memory pointed to
(lifetime
> of program).
> 
> You have to do the reinterpret_cast because the signed-ness of char is up to
the
> compiler/platform, the data you're pointing to may be a signed char[] or it
may
> be an unsigned char[]. It is safe to reinterpret cast because what you're
> pointing to is guaranteed to have the lifetime of the program and the elements
> of the array have the correct size_of. The signed/unsigned just affects what
> value you interpret these bytes as having.

Got it.
Thanks!

[ehmaldonado_webrtc, 2017-07-18 13:55:58]

The CQ bit was checked by ehmaldonado@webrtc.org

[ehmaldonado_webrtc, 2017-07-18 13:55:58]

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/2979203002/#ps60001 (title:
"Address Comments 2.")

[commit-bot: I haz the power, 2017-07-18 13:56:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2017-07-18 14:02:54]

lgtm.

I'll leave it up to you if you think it's worth the extra work, but if you feel
like it you could verify that AddTraceEventHandler is called for each stat in a
separate TEST_F where the handler adds to trace counters when it sees
"webrtc_stats" traces.

[commit-bot: I haz the power, 2017-07-18 14:35:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500386158246690,
"parent_rev": "b878575bec6d9c74e181e3c0beb473ef571a7075", "commit_rev":
"80c829f25382f98e97b30a5803322552f350f983"}

[commit-bot: I haz the power, 2017-07-18 14:35:24]

Description was changed from

==========
Enable tracing on rtcstats_integrationtest.cc

BUG=chromium:653087
==========

to

==========
Enable tracing on rtcstats_integrationtest.cc

BUG=chromium:653087

Review-Url: https://codereview.webrtc.org/2979203002
Cr-Commit-Position: refs/heads/master@{#19076}
Committed:
https://chromium.googlesource.com/external/webrtc/+/80c829f25382f98e97b30a580...
==========

[commit-bot: I haz the power, 2017-07-18 14:35:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/80c829f25382f98e97b30a580...

[ehmaldonado_webrtc, 2017-07-18 14:37:49]

On 2017/07/18 14:02:54, hbos wrote:
> lgtm.
> 
> I'll leave it up to you if you think it's worth the extra work, but if you
feel
> like it you could verify that AddTraceEventHandler is called for each stat in
a
> separate TEST_F where the handler adds to trace counters when it sees
> "webrtc_stats" traces.

I'll take a look :)