Use RTC_UNUSED instead of conditional compilation in BWE simulator tool.

Use RTC_UNUSED instead of conditional compilation in BWE simulator tool.

Mark ATTRIBUTE_UNUSED as deprecated since it only works with GCC and clang. I am not removing it now since typedefs.h is (perhaps incorrectly?) considered a public interface.


BUG=webrtc:7228
Review-Url: https://codereview.webrtc.org/2756483002
Cr-Commit-Position: refs/heads/master@{#17291}
Committed: https://chromium.googlesource.com/external/webrtc/+/a2bb667a9bc52f85b3ce5757aca37a9d02073f01

[terelius, 2017-03-15 16:25:51]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 16:25:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[terelius, 2017-03-15 16:26:24]

terelius@webrtc.org changed reviewers:
+ nisse@webrtc.org, stefan@webrtc.org

[commit-bot: I haz the power, 2017-03-15 16:30:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 16:30:27]

Dry run: Try jobs failed on following builders:
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)

[nisse-webrtc, 2017-03-16 08:06:51]

lgtm

In principle, I think that when this logging is disabled at compile time, *all*
code that the logging depends on ought to be taken out at compile time too. But
that's a larger change for a different cl.

If you see any reasonable and concrete way to reorganize, 
it would be nice to document in a bug or TODO item.

[terelius, 2017-03-16 12:44:48]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 12:44:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-16 12:52:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 12:52:48]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)

[terelius, 2017-03-16 13:34:08]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 13:34:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-16 13:39:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 13:39:45]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[terelius, 2017-03-16 13:50:52]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 13:50:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-16 14:28:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 14:28:31]

Dry run: This issue passed the CQ dry run.

[terelius, 2017-03-17 09:49:08]

Description was changed from

==========
Use ATTRIBUTE_UNUSED instead of conditional compilation in BWE simulator tool.

BUG=webrtc:7228
==========

to

==========
Use RTC_UNUSED instead of conditional compilation in BWE simulator tool.

Mark ATTRIBUTE_UNUSED as deprecated since it only works with GCC and clang. I am
not removing it now since typedefs.h is (perhaps incorrectly?) considered a
public interface.


BUG=webrtc:7228
==========

[terelius, 2017-03-17 09:50:44]

The CL now passes the trybots. Please take another look.

[nisse-webrtc, 2017-03-17 10:16:37]

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h
File webrtc/typedefs.h (right):

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode116
webrtc/typedefs.h:116: //   assert(result == 17);
I think the example is misleading, we don't use assert, and there's no need to
use RTC_UNUSED together with RTC_DCHECK.

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode119
webrtc/typedefs.h:119: #define RTC_UNUSED(x) RtcUnused(static_cast<const
void*>(&x))
Please try out this simpler definition:

  #define RTC_UNUSED(x) (static_cast<void>(x))

[terelius, 2017-03-17 10:28:55]

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h
File webrtc/typedefs.h (right):

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode116
webrtc/typedefs.h:116: //   assert(result == 17);
On 2017/03/17 10:16:37, nisse-webrtc wrote:
> I think the example is misleading, we don't use assert, and there's no need to
> use RTC_UNUSED together with RTC_DCHECK.

I based this on the example for ATTRIBUTE_UNUSED. Do you have any better
suggestion?

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode119
webrtc/typedefs.h:119: #define RTC_UNUSED(x) RtcUnused(static_cast<const
void*>(&x))
On 2017/03/17 10:16:37, nisse-webrtc wrote:
> Please try out this simpler definition:
> 
>   #define RTC_UNUSED(x) (static_cast<void>(x))

Already tried. It does not work on Android bots.

[nisse-webrtc, 2017-03-17 10:41:42]

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h
File webrtc/typedefs.h (right):

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode116
webrtc/typedefs.h:116: //   assert(result == 17);
On 2017/03/17 10:28:54, terelius wrote:
> On 2017/03/17 10:16:37, nisse-webrtc wrote:
> > I think the example is misleading, we don't use assert, and there's no need
to
> > use RTC_UNUSED together with RTC_DCHECK.
> 
> I based this on the example for ATTRIBUTE_UNUSED. Do you have any better
> suggestion?

There's rarely a good reason to use RTC_UNUSED, so maybe it's best to just
delete the example.

If you want an example, maybe you can cook one up from how it's actually used? 

int foo = get_foo();
FOO_LOGGING(foo);  // Expanding to empty in some builds
RTC_UNUSED(foo);

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode119
webrtc/typedefs.h:119: #define RTC_UNUSED(x) RtcUnused(static_cast<const
void*>(&x))
On 2017/03/17 10:28:54, terelius wrote:
> On 2017/03/17 10:16:37, nisse-webrtc wrote:
> > Please try out this simpler definition:
> > 
> >   #define RTC_UNUSED(x) (static_cast<void>(x))
> 
> Already tried. It does not work on Android bots.

Where's the failure? I see you tried 

#define RTC_UNUSED(x) static_cast<const void*>(&x)

in patchset #2, but that's quite different from what I'm suggesting.

[terelius, 2017-03-17 11:03:27]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 11:03:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-17 11:39:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 11:39:13]

Dry run: This issue passed the CQ dry run.

[terelius, 2017-03-17 11:59:15]

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h
File webrtc/typedefs.h (right):

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode116
webrtc/typedefs.h:116: //   assert(result == 17);
On 2017/03/17 10:41:42, nisse-webrtc wrote:
> On 2017/03/17 10:28:54, terelius wrote:
> > On 2017/03/17 10:16:37, nisse-webrtc wrote:
> > > I think the example is misleading, we don't use assert, and there's no
need
> to
> > > use RTC_UNUSED together with RTC_DCHECK.
> > 
> > I based this on the example for ATTRIBUTE_UNUSED. Do you have any better
> > suggestion?
> 
> There's rarely a good reason to use RTC_UNUSED, so maybe it's best to just
> delete the example.
> 
> If you want an example, maybe you can cook one up from how it's actually used?

> 
> int foo = get_foo();
> FOO_LOGGING(foo);  // Expanding to empty in some builds
> RTC_UNUSED(foo);

I like having an example that shows how to use the macro and in particular where
it should be placed compared to ATTRIBUTE_UNUSED. I also prefer using constructs
that'll be familiar to the reader (e.g. assert) over more obscure logging macros
in the example.

I have added a note in the comment that discourages use of RTC_UNUSED though.

https://codereview.webrtc.org/2756483002/diff/60001/webrtc/typedefs.h#newcode119
webrtc/typedefs.h:119: #define RTC_UNUSED(x) RtcUnused(static_cast<const
void*>(&x))
On 2017/03/17 10:41:42, nisse-webrtc wrote:
> On 2017/03/17 10:28:54, terelius wrote:
> > On 2017/03/17 10:16:37, nisse-webrtc wrote:
> > > Please try out this simpler definition:
> > > 
> > >   #define RTC_UNUSED(x) (static_cast<void>(x))
> > 
> > Already tried. It does not work on Android bots.
> 
> Where's the failure? I see you tried 
> 
> #define RTC_UNUSED(x) static_cast<const void*>(&x)
> 
> in patchset #2, but that's quite different from what I'm suggesting.

Sorry, I misread your comment. Yeah, that seems to work.

[nisse-webrtc, 2017-03-17 12:07:16]

lgtm (again).

[stefan-webrtc, 2017-03-17 13:18:12]

lgtm

[terelius, 2017-03-17 13:32:48]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2017-03-17 13:32:53]

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

[commit-bot: I haz the power, 2017-03-17 13:34:48]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1489757568324050,
"parent_rev": "dd20054e06e18663a306df4dd8029d11006b8c05", "commit_rev":
"a2bb667a9bc52f85b3ce5757aca37a9d02073f01"}

[commit-bot: I haz the power, 2017-03-17 13:34:50]

Description was changed from

==========
Use RTC_UNUSED instead of conditional compilation in BWE simulator tool.

Mark ATTRIBUTE_UNUSED as deprecated since it only works with GCC and clang. I am
not removing it now since typedefs.h is (perhaps incorrectly?) considered a
public interface.


BUG=webrtc:7228
==========

to

==========
Use RTC_UNUSED instead of conditional compilation in BWE simulator tool.

Mark ATTRIBUTE_UNUSED as deprecated since it only works with GCC and clang. I am
not removing it now since typedefs.h is (perhaps incorrectly?) considered a
public interface.


BUG=webrtc:7228

Review-Url: https://codereview.webrtc.org/2756483002
Cr-Commit-Position: refs/heads/master@{#17291}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a2bb667a9bc52f85b3ce5757a...
==========

[commit-bot: I haz the power, 2017-03-17 13:34:52]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/external/webrtc/+/a2bb667a9bc52f85b3ce5757a...