Enable the -Wundef warning for clang

Enable the -Wundef warning for clang

NOPRESUBMIT=true
BUG=webrtc:6398
Committed: https://crrev.com/77eab704707e03c74eda3126b92e4ef5e2b7eb4d
Cr-Commit-Position: refs/heads/master@{#14425}

[kwiberg-webrtc, 2016-09-23 08:02:10]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-09-23 08:03:41]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:04:35]

Patchset #1 (id:40001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:06:11]

Patchset #1 (id:60001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:07:55]

Patchset #1 (id:80001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:08:35]

Patchset #1 (id:100001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:09:14]

Patchset #1 (id:120001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:09:53]

Patchset #1 (id:140001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:12:54]

Patchset #1 (id:160001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:15:00]

Patchset #1 (id:180001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:18:08]

Patchset #1 (id:200001) has been deleted

[kwiberg-webrtc, 2016-09-23 08:54:46]

kwiberg@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kwiberg-webrtc, 2016-09-23 08:54:49]

I thought I could fix the couple of things standing between us and -Wundef, but
more things kept popping up... anyway, it's done now. :-)

Patch set #1 is just enabling -Wundef. #2 and #3 are automatic #include rewrites
(I added an indirection when people try to include gtest.h and gmock.h). #4 are
the manual changes I had to make to get things to compile.

[kjellander_webrtc, 2016-09-23 12:32:38]

Awesome work!

Please remove the +testing rule from:
https://cs.chromium.org/chromium/src/third_party/webrtc/DEPS?rcl=0&l=10

And add the new headers to the rtc_base_approved target in
webrtc/base/{base.gyp,BUILD.gn}

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:15: #include
"webrtc/base/gmock.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/api/rtpsenderrece...
File webrtc/api/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/api/rtpsenderrece...
webrtc/api/rtpsenderreceiver_unittest.cc:15: #include "webrtc/base/gmock.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect.cc
File webrtc/base/proxydetect.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect....
webrtc/base/proxydetect.cc:48: #define _TRY_JSPROXY 0
_TRY_JSPROXY 1 ?

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect....
webrtc/base/proxydetect.cc:49: #define _TRY_WM_FINDPROXY 0
_TRY_WM_FINDPROXY 1 ?

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/sequenced_ta...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:10: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/thread_check...
File webrtc/base/thread_checker_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/thread_check...
webrtc/base/thread_checker_unittest.cc:15: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:15: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:16: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/call_unittes...
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/call_unittes...
webrtc/call/call_unittest.cc:14: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/mock/mock_rt...
File webrtc/call/mock/mock_rtc_event_log.h (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/mock/mock_rt...
webrtc/call/mock/mock_rtc_event_log.h:17: 
remove blank line?

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/packet_injec...
File webrtc/call/packet_injection_tests.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/packet_injec...
webrtc/call/packet_injection_tests.cc:14: 
remove blank line?

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/rampup_tests...
webrtc/call/rampup_tests.cc:13: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/rtc_event_lo...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/rtc_event_lo...
webrtc/call/rtc_event_log_unittest.cc:17: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/rtc_event_lo...
File webrtc/call/rtc_event_log_unittest_helper.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/call/rtc_event_lo...
webrtc/call/rtc_event_log_unittest_helper.cc:17: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/audi...
File webrtc/common_audio/audio_converter_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/audi...
webrtc/common_audio/audio_converter_unittest.cc:16: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/bloc...
File webrtc/common_audio/blocker_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/bloc...
webrtc/common_audio/blocker_unittest.cc:15: #include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/resa...
File webrtc/common_audio/resampler/push_resampler_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/resa...
webrtc/common_audio/resampler/push_resampler_unittest.cc:11: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/resa...
File webrtc/common_audio/resampler/resampler_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/resa...
webrtc/common_audio/resampler/resampler_unittest.cc:12: 
remove blank line

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/sign...
File webrtc/common_audio/signal_processing/real_fft_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/sign...
webrtc/common_audio/signal_processing/real_fft_unittest.cc:14: 
remove blank line + sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/vad/...
File webrtc/common_audio/vad/vad_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/vad/...
webrtc/common_audio/vad/vad_unittest.cc:16: 
remove blank line + sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/vad/...
File webrtc/common_audio/vad/vad_unittest.h (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_audio/vad/...
webrtc/common_audio/vad/vad_unittest.h:17: 
remove blank line

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_video/i420...
File webrtc/common_video/i420_video_frame_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/common_video/i420...
webrtc/common_video/i420_video_frame_unittest.cc:14: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/media/base/test/m...
File webrtc/media/base/test/mock_mediachannel.h (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/media/base/test/m...
webrtc/media/base/test/mock_mediachannel.h:15: #include "webrtc/base/gmock.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/media/engine/null...
File webrtc/media/engine/nullwebrtcvideoengine_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/media/engine/null...
webrtc/media/engine/nullwebrtcvideoengine_unittest.cc:12: 
remove blank line.

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:16: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/acm_send_test_oldapi.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/acm_send_test_oldapi.cc:17: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc:16:
#include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:14:
#include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:15:
#include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:18:
#include "webrtc/base/gmock.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc:14:
#include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:18: 
remove blank line + sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:13:
#include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:13: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:14: #include
"webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc
(right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc:14:
#include "webrtc/base/gtest.h"
sort

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc:13: #include
"webrtc/base/gtest.h"
sort

[kwiberg-webrtc, 2016-09-26 08:34:31]

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect.cc
File webrtc/base/proxydetect.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect....
webrtc/base/proxydetect.cc:48: #define _TRY_JSPROXY 0
On 2016/09/23 12:32:36, kjellander_webrtc wrote:
> _TRY_JSPROXY 1 ?

It used to default to 0 (unless someone was setting it from outside...), so
setting it to anything but 0 seems like out of scope for this CL.

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect....
webrtc/base/proxydetect.cc:49: #define _TRY_WM_FINDPROXY 0
On 2016/09/23 12:32:36, kjellander_webrtc wrote:
> _TRY_WM_FINDPROXY 1 ?

Same.

[kjellander_webrtc, 2016-09-26 08:38:14]

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect.cc
File webrtc/base/proxydetect.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect....
webrtc/base/proxydetect.cc:48: #define _TRY_JSPROXY 0
On 2016/09/26 08:34:30, kwiberg-webrtc wrote:
> On 2016/09/23 12:32:36, kjellander_webrtc wrote:
> > _TRY_JSPROXY 1 ?
> 
> It used to default to 0 (unless someone was setting it from outside...), so
> setting it to anything but 0 seems like out of scope for this CL.

OK, but then you can move them outside the
#if defined(WEBRTC_WIN)
section at least.

[kwiberg-webrtc, 2016-09-26 08:42:57]

On 2016/09/23 12:32:38, kjellander_webrtc wrote:
> Awesome work!
> 
> Please remove the +testing rule from:
> https://cs.chromium.org/chromium/src/third_party/webrtc/DEPS?rcl=0&l=10
> 
> And add the new headers to the rtc_base_approved target in
> webrtc/base/{base.gyp,BUILD.gn}

New patch set uploaded with these things fixed.

Sorting the headers (and eliminating blank lines) is going to make this CL
attract merge conflicts like flies, and unlike the renaming I don't have a tool
that'll re-do the change for me automatically. Can I have an LGTM in advance, so
that I can try to land this the minute I'm done sorting? (Which I'll put off
until the commit queue is healthy again.)

[kwiberg-webrtc, 2016-09-26 08:44:26]

On 2016/09/26 08:42:57, kwiberg-webrtc wrote:
> On 2016/09/23 12:32:38, kjellander_webrtc wrote:
> > Awesome work!
> > 
> > Please remove the +testing rule from:
> > https://cs.chromium.org/chromium/src/third_party/webrtc/DEPS?rcl=0&l=10
> > 
> > And add the new headers to the rtc_base_approved target in
> > webrtc/base/{base.gyp,BUILD.gn}
> 
> New patch set uploaded with these things fixed.

Wait, it failed. Because now webrtc/base/gmock.h and /gtest.h aren't allowed to
#include stuff from testing/...

[kwiberg-webrtc, 2016-09-26 08:48:52]

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect.cc
File webrtc/base/proxydetect.cc (right):

https://codereview.webrtc.org/2358993004/diff/280001/webrtc/base/proxydetect....
webrtc/base/proxydetect.cc:48: #define _TRY_JSPROXY 0
On 2016/09/26 08:38:14, kjellander_webrtc wrote:
> On 2016/09/26 08:34:30, kwiberg-webrtc wrote:
> > On 2016/09/23 12:32:36, kjellander_webrtc wrote:
> > > _TRY_JSPROXY 1 ?
> > 
> > It used to default to 0 (unless someone was setting it from outside...), so
> > setting it to anything but 0 seems like out of scope for this CL.
> 
> OK, but then you can move them outside the
> #if defined(WEBRTC_WIN)
> section at least.

Sure.

[kjellander_webrtc, 2016-09-26 08:57:00]

On 2016/09/26 08:44:26, kwiberg-webrtc wrote:
> On 2016/09/26 08:42:57, kwiberg-webrtc wrote:
> > On 2016/09/23 12:32:38, kjellander_webrtc wrote:
> > > Awesome work!
> > > 
> > > Please remove the +testing rule from:
> > > https://cs.chromium.org/chromium/src/third_party/webrtc/DEPS?rcl=0&l=10
> > > 
> > > And add the new headers to the rtc_base_approved target in
> > > webrtc/base/{base.gyp,BUILD.gn}
> > 
> > New patch set uploaded with these things fixed.
> 
> Wait, it failed. Because now webrtc/base/gmock.h and /gtest.h aren't allowed
to
> #include stuff from testing/...

Just include this in webrtc/base/DEPS to whitelist them to use testing:

specific_include_rules = {
  "gmock\.h": [
    "+testing/gmock/include/gmock",
  ],
  "gtest\.h": [
    "+testing/gtest/include/gtest",
  ],
}


I guess you might need something for gunit.h as well.

[kwiberg-webrtc, 2016-09-26 09:10:29]

On 2016/09/26 08:57:00, kjellander_webrtc wrote:
> I guess you might need something for gunit.h as well.

It doesn't seem that way. I'll give it to the bots for a full run and see if
they spot anything that didn't fail for me locally.

[kwiberg-webrtc, 2016-09-26 10:48:06]

I get errors of the type

The target:
  //webrtc/modules/audio_device:audio_device_tests
is including a file from the target:
  //webrtc/base:rtc_base_approved

I guess the right thing to do would be to add a dependency on rtc_base_approved
in such cases?

Hmm. Shouldn't I also add a dependency *from* rtc_base_approved on whatever
build target gtest and gmock live in? And since such a dependency feels
inappropriate for rtc_base_approved, this probably means that gtest.h and
gmock.h should be in their own build target(s)... What do you think?

[kjellander_webrtc, 2016-09-26 11:43:12]

On 2016/09/26 10:48:06, kwiberg-webrtc wrote:
> I get errors of the type
> 
> The target:
>   //webrtc/modules/audio_device:audio_device_tests
> is including a file from the target:
>   //webrtc/base:rtc_base_approved
> 
> I guess the right thing to do would be to add a dependency on
rtc_base_approved
> in such cases?
> 
> Hmm. Shouldn't I also add a dependency *from* rtc_base_approved on whatever
> build target gtest and gmock live in? And since such a dependency feels
> inappropriate for rtc_base_approved, this probably means that gtest.h and
> gmock.h should be in their own build target(s)... What do you think?

Oh. I didn't really pay attention to this. You're absolutely right - the headers
shouldn't even be in webrtc/base/ IMO, since that's production code while this
is test utilities. They should be in a target that is marked as testonly = true,
which in turn has dependencies on gtest and gmock.
What about moving them to webrtc/test/ or webrtc/test/test_support? I like the
shorter path better but the "test_support" target seems like a much more natural
choice of a target.
Ideally we'd refactor all the contents of webrtc/test into a more logical
directory and target structure, then put these two headers in 'test_support'.
WDYT? I could help with that.

[kwiberg-webrtc, 2016-09-26 12:19:56]

On 2016/09/26 11:43:12, kjellander_webrtc wrote:
> On 2016/09/26 10:48:06, kwiberg-webrtc wrote:
> > I get errors of the type
> > 
> > The target:
> >   //webrtc/modules/audio_device:audio_device_tests
> > is including a file from the target:
> >   //webrtc/base:rtc_base_approved
> > 
> > I guess the right thing to do would be to add a dependency on
> rtc_base_approved
> > in such cases?
> > 
> > Hmm. Shouldn't I also add a dependency *from* rtc_base_approved on whatever
> > build target gtest and gmock live in? And since such a dependency feels
> > inappropriate for rtc_base_approved, this probably means that gtest.h and
> > gmock.h should be in their own build target(s)... What do you think?
> 
> Oh. I didn't really pay attention to this. You're absolutely right - the
headers
> shouldn't even be in webrtc/base/ IMO, since that's production code while this
> is test utilities. They should be in a target that is marked as testonly =
true,
> which in turn has dependencies on gtest and gmock.
> What about moving them to webrtc/test/ or webrtc/test/test_support? I like the
> shorter path better but the "test_support" target seems like a much more
natural
> choice of a target.
> Ideally we'd refactor all the contents of webrtc/test into a more logical
> directory and target structure, then put these two headers in 'test_support'.
> WDYT? I could help with that.

Hmm. What's the distinction between the webrtc/test/ and
webrtc/test/testsupport/ stuff anyway? Is there even a point of having it in
different directories? (If there is, I'm all for it. But I'm not that familiar
with this code.)

I can't commit to spending much more time on this at the moment, unfortunately.
Ideally, I'd like to land this CL in a way that puts the new gmock.h and gtest.h
in the directory where this refactoring would eventually put them, so that we
don't have to PSA a new location twice. Would that make things much harder?

[kjellander_webrtc, 2016-09-27 06:07:09]

Description was changed from

==========
Enable the -Wundef warning for clang

BUG=webrtc:6398
==========

to

==========
Enable the -Wundef warning for clang

BUG=webrtc:6398
==========

[kjellander_webrtc, 2016-09-27 06:07:09]

kjellander@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[kjellander_webrtc, 2016-09-27 06:07:33]

On 2016/09/26 12:19:56, kwiberg-webrtc wrote:
> On 2016/09/26 11:43:12, kjellander_webrtc wrote:
> > On 2016/09/26 10:48:06, kwiberg-webrtc wrote:
> > > I get errors of the type
> > > 
> > > The target:
> > >   //webrtc/modules/audio_device:audio_device_tests
> > > is including a file from the target:
> > >   //webrtc/base:rtc_base_approved
> > > 
> > > I guess the right thing to do would be to add a dependency on
> > rtc_base_approved
> > > in such cases?
> > > 
> > > Hmm. Shouldn't I also add a dependency *from* rtc_base_approved on
whatever
> > > build target gtest and gmock live in? And since such a dependency feels
> > > inappropriate for rtc_base_approved, this probably means that gtest.h and
> > > gmock.h should be in their own build target(s)... What do you think?
> > 
> > Oh. I didn't really pay attention to this. You're absolutely right - the
> headers
> > shouldn't even be in webrtc/base/ IMO, since that's production code while
this
> > is test utilities. They should be in a target that is marked as testonly =
> true,
> > which in turn has dependencies on gtest and gmock.
> > What about moving them to webrtc/test/ or webrtc/test/test_support? I like
the
> > shorter path better but the "test_support" target seems like a much more
> natural
> > choice of a target.
> > Ideally we'd refactor all the contents of webrtc/test into a more logical
> > directory and target structure, then put these two headers in
'test_support'.
> > WDYT? I could help with that.
> 
> Hmm. What's the distinction between the webrtc/test/ and
> webrtc/test/testsupport/ stuff anyway? Is there even a point of having it in
> different directories? (If there is, I'm all for it. But I'm not that familiar
> with this code.)

I don't remember. I think initially we only had subdirectories in webrtc/test
and kept the GYP files in that dir. Then people started putting more and more .h
and .cc files directly in webrtc/test, which may or may not make sense. I like
the shorter include path webrtc/test/gtest.h for something that will be so
commonly included.
 
> I can't commit to spending much more time on this at the moment,
unfortunately.
> Ideally, I'd like to land this CL in a way that puts the new gmock.h and
gtest.h
> in the directory where this refactoring would eventually put them, so that we
> don't have to PSA a new location twice. Would that make things much harder?

I totally understand that, you've spent a lot of time on this already. I suggest
you just put gtest.h and gmock.h in webrtc/test for now and include them in the
testsupport target.
+phoglund for any other comments regarding the locations of these files.

[kwiberg-webrtc, 2016-09-27 09:19:17]

Patchset #7 (id:340001) has been deleted

[kwiberg-webrtc, 2016-09-27 09:21:32]

Patchset #7 (id:360001) has been deleted

[phoglund, 2016-09-27 09:23:42]

On 2016/09/27 06:07:33, kjellander_webrtc wrote:
> On 2016/09/26 12:19:56, kwiberg-webrtc wrote:
> > On 2016/09/26 11:43:12, kjellander_webrtc wrote:
> > > On 2016/09/26 10:48:06, kwiberg-webrtc wrote:
> > > > I get errors of the type
> > > > 
> > > > The target:
> > > >   //webrtc/modules/audio_device:audio_device_tests
> > > > is including a file from the target:
> > > >   //webrtc/base:rtc_base_approved
> > > > 
> > > > I guess the right thing to do would be to add a dependency on
> > > rtc_base_approved
> > > > in such cases?
> > > > 
> > > > Hmm. Shouldn't I also add a dependency *from* rtc_base_approved on
> whatever
> > > > build target gtest and gmock live in? And since such a dependency feels
> > > > inappropriate for rtc_base_approved, this probably means that gtest.h
and
> > > > gmock.h should be in their own build target(s)... What do you think?
> > > 
> > > Oh. I didn't really pay attention to this. You're absolutely right - the
> > headers
> > > shouldn't even be in webrtc/base/ IMO, since that's production code while
> this
> > > is test utilities. They should be in a target that is marked as testonly =
> > true,
> > > which in turn has dependencies on gtest and gmock.
> > > What about moving them to webrtc/test/ or webrtc/test/test_support? I like
> the
> > > shorter path better but the "test_support" target seems like a much more
> > natural
> > > choice of a target.
> > > Ideally we'd refactor all the contents of webrtc/test into a more logical
> > > directory and target structure, then put these two headers in
> 'test_support'.
> > > WDYT? I could help with that.
> > 
> > Hmm. What's the distinction between the webrtc/test/ and
> > webrtc/test/testsupport/ stuff anyway? Is there even a point of having it in
> > different directories? (If there is, I'm all for it. But I'm not that
familiar
> > with this code.)
> 
> I don't remember. I think initially we only had subdirectories in webrtc/test
> and kept the GYP files in that dir. Then people started putting more and more
.h
> and .cc files directly in webrtc/test, which may or may not make sense. I like
> the shorter include path webrtc/test/gtest.h for something that will be so
> commonly included.
>  
> > I can't commit to spending much more time on this at the moment,
> unfortunately.
> > Ideally, I'd like to land this CL in a way that puts the new gmock.h and
> gtest.h
> > in the directory where this refactoring would eventually put them, so that
we
> > don't have to PSA a new location twice. Would that make things much harder?
> 
> I totally understand that, you've spent a lot of time on this already. I
suggest
> you just put gtest.h and gmock.h in webrtc/test for now and include them in
the
> testsupport target.
> +phoglund for any other comments regarding the locations of these files.

I think webrtc/test/gmock.h looks strange since gmock is a third-party library,
but I can accept that because test frameworks are necessarily tightly coupled to
the tests, anyway. I don't think this should cause any problems downstream
either, so I say go for it.

[kwiberg-webrtc, 2016-09-27 09:26:04]

On 2016/09/27 09:23:42, phoglund wrote:
> I think webrtc/test/gmock.h looks strange since gmock is a third-party
library,
> but I can accept that because test frameworks are necessarily tightly coupled
to
> the tests, anyway. I don't think this should cause any problems downstream
> either, so I say go for it.

Yes, I think it makes sense since it's a wrapper around the actual #include of
the third-party header in question.

[kwiberg-webrtc, 2016-09-27 09:49:31]

Uploaded two new patch sets that do everything from scratch (necessary because
rebases, and because I had to re-do the scripted find-and-replace operations
when I moved the new headers to webrtc/test/). For patch set #7 (all the
scripted changes), look at the diff against the base; for patch set #8 (all the
manual changes), look at the diff against patch set #7.

Given the amount of merge conflicts I'm getting just by keeping this version of
the CL current, I'd prefer to sort the includes in a follow-up CL, since that's
going to touch even more lines. With a separate CL, I have the option of
splitting it up into several CLs if necessary.

[kjellander_webrtc, 2016-09-27 13:02:15]

On 2016/09/27 09:49:31, kwiberg-webrtc wrote:
> Uploaded two new patch sets that do everything from scratch (necessary because
> rebases, and because I had to re-do the scripted find-and-replace operations
> when I moved the new headers to webrtc/test/). For patch set #7 (all the
> scripted changes), look at the diff against the base; for patch set #8 (all
the
> manual changes), look at the diff against patch set #7.

lgtm, hurry to commit it (but find a root owner to rubberstamp).

> Given the amount of merge conflicts I'm getting just by keeping this version
of
> the CL current, I'd prefer to sort the includes in a follow-up CL, since
that's
> going to touch even more lines. With a separate CL, I have the option of
> splitting it up into several CLs if necessary.

That's fine with me.

[kwiberg-webrtc, 2016-09-29 00:09:23]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-09-29 00:09:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2358993004/#ps420001 (title:
"rebase")

[commit-bot: I haz the power, 2016-09-29 00:09:29]

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, 2016-09-29 00:28:15]

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

[commit-bot: I haz the power, 2016-09-29 00:28:19]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8773)

[kwiberg-webrtc, 2016-09-29 00:39:42]

Description was changed from

==========
Enable the -Wundef warning for clang

BUG=webrtc:6398
==========

to

==========
Enable the -Wundef warning for clang

NOPRESUBMIT=true
BUG=webrtc:6398
==========

[kwiberg-webrtc, 2016-09-29 00:40:01]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-09-29 00:40:15]

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, 2016-09-29 00:42:07]

Description was changed from

==========
Enable the -Wundef warning for clang

NOPRESUBMIT=true
BUG=webrtc:6398
==========

to

==========
Enable the -Wundef warning for clang

NOPRESUBMIT=true
BUG=webrtc:6398
==========

[commit-bot: I haz the power, 2016-09-29 00:42:09]

Committed patchset #9 (id:420001)

[commit-bot: I haz the power, 2016-09-29 00:42:16]

Description was changed from

==========
Enable the -Wundef warning for clang

NOPRESUBMIT=true
BUG=webrtc:6398
==========

to

==========
Enable the -Wundef warning for clang

NOPRESUBMIT=true
BUG=webrtc:6398

Committed: https://crrev.com/77eab704707e03c74eda3126b92e4ef5e2b7eb4d
Cr-Commit-Position: refs/heads/master@{#14425}
==========

[commit-bot: I haz the power, 2016-09-29 00:42:19]

Patchset 9 (id:??) landed as
https://crrev.com/77eab704707e03c74eda3126b92e4ef5e2b7eb4d
Cr-Commit-Position: refs/heads/master@{#14425}