Delete webrtc/base/common.h

Delete webrtc/base/common.h

BUG=webrtc:6424
Review-Url: https://codereview.webrtc.org/2684613002
Cr-Commit-Position: refs/heads/master@{#16718}
Committed: https://chromium.googlesource.com/external/webrtc/+/21e4e0b0ab1ea630a9b1ada3d9be576f1073469d

[nisse-webrtc, 2017-02-08 08:43:41]

Description was changed from

==========
Delete most of webrtc/base/common.h

BUG=webrtc:6424
==========

to

==========
Delete webrtc/base/common.h

BUG=webrtc:6424
==========

[nisse-webrtc, 2017-02-08 09:03:51]

nisse@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, pthatcher@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-02-08 09:03:52]

This cl deletes webrtc/base/common.h. The bulk of that code was the
ASSERT/VERIFY framework which I've purged all use of in a couple of earlier cls.


It also defined RTC_UNUSED and stricmp. I'm not entirely satisfied with
replacements in this cl.

First, I think we ought to have a single header file with all system and
compiler dependent definitions. E.g.,  stricmp (windows) vs strcasecmp (posix).
And definitions related to __attribute__. This could be base/common.h, even if
that is a poor name.

For the first version of this patchset, stricmp is replaced by STR_CASE_CMP, and
where needed, an additional include of webrtc/common_types.h which defines this
wrapper macro.

Most cases of RTC_UNUSED were deleted. In a few places, replaced by
ATTRIBUTE_UNUSED, defined in webrtc/typedefs.h. 

For the BWE logging, I instead added preprocessor test on
BWE_TEST_LOGGING_COMPILE_TIME_ENABLE where needed. Maybe not that pretty, but
when compilation of this code is disabled, I think it is preferable to take out
complete functions, not only the calls to
webrtc::testing::bwe::Logging::GetInstance()

For unusedness warnings, some options are:

 * keep RTC_UNUSED as is, but moved if we want to get rid of common.h? 

 * Use ATTRIBUTE_UNUSED consistently (but move/rename?)

 * If there are few uses, it may be best to keep it really simple and just write
an explicit cast to (void) where needed.

Note that RTC_UNUSED and ATTRIBUTE_UNUSED aren't equivalent, the former is
inserted into a code block, while the latter is attached to the declaration of
the unused variable.

My experience with __attribute__ ((unused)) is that the compiler doesn't
complain if a variable is marked as unused, but nonetheless used in the code.

[kwiberg-webrtc, 2017-02-09 09:48:43]

If you're sure that no one external is #including the header files you're
removing, I guess doing it all in one step is OK. Alternatively, remove usage
and the actual files in separate CLs, so that you'll only have to roll back the
removal if you break users.

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:253: int
flow_id_ ATTRIBUTE_UNUSED;
Why not remove it if it's unused? (Or if it's used conditionally, #ifdef this
line on the same condition?)

I'm asking because I suspect we can manage to not have an UNUSED macro at all.

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h (right):

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h:73: bool
plot_estimate_ ATTRIBUTE_UNUSED;
#if instead?

[kwiberg-webrtc, 2017-02-09 09:59:52]

On 2017/02/08 09:03:52, nisse-webrtc wrote:
> This cl deletes webrtc/base/common.h. The bulk of that code was the
> ASSERT/VERIFY framework which I've purged all use of in a couple of earlier
cls.
> 
> 
> It also defined RTC_UNUSED and stricmp. I'm not entirely satisfied with
> replacements in this cl.
> 
> First, I think we ought to have a single header file with all system and
> compiler dependent definitions. E.g.,  stricmp (windows) vs strcasecmp
(posix).
> And definitions related to __attribute__. This could be base/common.h, even if
> that is a poor name.

Sounds good.

> For the first version of this patchset, stricmp is replaced by STR_CASE_CMP,
and
> where needed, an additional include of webrtc/common_types.h which defines
this
> wrapper macro.

Tangent: If you clean this up further, consider making it a string equality
function that accepts both const char* and const std::string&, and returns bool.

> Most cases of RTC_UNUSED were deleted. In a few places, replaced by
> ATTRIBUTE_UNUSED, defined in webrtc/typedefs.h. 
> 
> For the BWE logging, I instead added preprocessor test on
> BWE_TEST_LOGGING_COMPILE_TIME_ENABLE where needed. Maybe not that pretty, but
> when compilation of this code is disabled, I think it is preferable to take
out
> complete functions, not only the calls to
> webrtc::testing::bwe::Logging::GetInstance()
> 
> For unusedness warnings, some options are:
> 
>  * keep RTC_UNUSED as is, but moved if we want to get rid of common.h? 
> 
>  * Use ATTRIBUTE_UNUSED consistently (but move/rename?)
> 
>  * If there are few uses, it may be best to keep it really simple and just
write
> an explicit cast to (void) where needed.

I suspect we can do without a macro or (void) in almost all cases.

> Note that RTC_UNUSED and ATTRIBUTE_UNUSED aren't equivalent, the former is
> inserted into a code block, while the latter is attached to the declaration of
> the unused variable.
> 
> My experience with __attribute__ ((unused)) is that the compiler doesn't
> complain if a variable is marked as unused, but nonetheless used in the code.

Yes, that matches my understanding too: It's not a declaration that the variable
is unused, just that it's allowed to be unused.

[nisse-webrtc, 2017-02-09 11:26:18]

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:253: int
flow_id_ ATTRIBUTE_UNUSED;
On 2017/02/09 09:48:43, kwiberg-webrtc wrote:
> Why not remove it if it's unused? (Or if it's used conditionally, #ifdef this
> line on the same condition?)
> 
> I'm asking because I suspect we can manage to not have an UNUSED macro at all.

Changed to #if. It's mentioned in two constructors. One can be replaced with a
default value here, the other needs another #ifdef.

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h (right):

https://codereview.webrtc.org/2684613002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h:73: bool
plot_estimate_ ATTRIBUTE_UNUSED;
On 2017/02/09 09:48:43, kwiberg-webrtc wrote:
> #if instead?

Done.

[pthatcher1, 2017-02-10 23:44:17]

https://codereview.webrtc.org/2684613002/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/2684613002/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:255: #endif
I don't understand why BWE_TEST_LOGGING_COMPILE_TIME_ENABLE is part of this CL.

[kwiberg-webrtc, 2017-02-11 21:30:06]

https://codereview.webrtc.org/2684613002/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/2684613002/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:255: #endif
On 2017/02/10 23:44:17, pthatcher1 wrote:
> I don't understand why BWE_TEST_LOGGING_COMPILE_TIME_ENABLE is part of this
CL.

I think it is because this variable is only used by code protected by #if
BWE_TEST_LOGGING_COMPILE_TIME_ENABLE; we used to avoid unused variable warnings
by calling RTC_UNUSED(flow_id_), but now we do it like this instead.

nisse@, please correct me if I'm wrong...

[nisse-webrtc, 2017-02-13 12:05:51]

https://codereview.webrtc.org/2684613002/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h (right):

https://codereview.webrtc.org/2684613002/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.h:255: #endif
On 2017/02/11 21:30:06, kwiberg-webrtc wrote:
> On 2017/02/10 23:44:17, pthatcher1 wrote:
> > I don't understand why BWE_TEST_LOGGING_COMPILE_TIME_ENABLE is part of this
> CL.
> 
> I think it is because this variable is only used by code protected by #if
> BWE_TEST_LOGGING_COMPILE_TIME_ENABLE; we used to avoid unused variable
warnings
> by calling RTC_UNUSED(flow_id_), but now we do it like this instead.
> 
> nisse@, please correct me if I'm wrong...

You're right. Without these #ifdefs, I get warnings for unused variables, and
thanks to the use of -Werror, the warnings are fatal.

We could reintroduce RTC_UNUSED or something equivalent (an earlier patchset
used ATTRIBUTE_UNUSED at this location). But if the member variable is needed
only if BWE_... is true at compile time, I think it's good to make that
explicit.

It would perhaps be better to have the #ifdefs take out larger pieces of code,
preferably complete classes or complete files, but that's for some other cl.

[pthatcher1, 2017-02-13 17:42:34]

lgtm

[stefan-webrtc, 2017-02-17 10:27:32]

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

[stefan-webrtc, 2017-02-17 10:27:32]

+terelius fyi

https://codereview.webrtc.org/2684613002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/2684613002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:278: #if
BWE_TEST_LOGGING_COMPILE_TIME_ENABLE
I really would prefer a better option to this... Maybe what we should do is
change the plot logging to not use macros so that we can use a null
implementation.

Not for this CL though, but maybe you could add a TODO?

[stefan-webrtc, 2017-02-17 10:27:46]

lgtm % todo

[nisse-webrtc, 2017-02-20 08:11:22]

https://codereview.webrtc.org/2684613002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/2684613002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:278: #if
BWE_TEST_LOGGING_COMPILE_TIME_ENABLE
On 2017/02/17 10:27:32, stefan-webrtc wrote:
> I really would prefer a better option to this... Maybe what we should do is
> change the plot logging to not use macros so that we can use a null
> implementation.
> 
> Not for this CL though, but maybe you could add a TODO?

I added a TODO, with your name on it.

[nisse-webrtc, 2017-02-20 10:38:17]

I think this can be landed now, without downstream breakage. Agree?

[kwiberg-webrtc, 2017-02-20 11:47:38]

lgtm

[nisse-webrtc, 2017-02-20 12:16:39]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-20 12:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2684613002/#ps100001 (title:
"Added TODO comment.")

[commit-bot: I haz the power, 2017-02-20 12:16:49]

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-02-20 13:01:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487592999379660,
"parent_rev": "e5c27a5db6dab92176f90c4a0cd5a77c6d166a58", "commit_rev":
"21e4e0b0ab1ea630a9b1ada3d9be576f1073469d"}

[commit-bot: I haz the power, 2017-02-20 13:01:07]

Description was changed from

==========
Delete webrtc/base/common.h

BUG=webrtc:6424
==========

to

==========
Delete webrtc/base/common.h

BUG=webrtc:6424

Review-Url: https://codereview.webrtc.org/2684613002
Cr-Commit-Position: refs/heads/master@{#16718}
Committed:
https://chromium.googlesource.com/external/webrtc/+/21e4e0b0ab1ea630a9b1ada3d...
==========

[commit-bot: I haz the power, 2017-02-20 13:01:08]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/21e4e0b0ab1ea630a9b1ada3d...