test IWYU fixups for base

test IWYU fixups for base

These are some of the suggestions from running include-what-you-use[1]
on the base target.

[1] https://code.google.com/p/include-what-you-use/

BUG=259043
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221067

[Mostyn Bramley-Moore, 2013-08-29 13:37:19]

@Nico: does this patch look OK to you?

I am experimenting with include-what-you-use (IWYU) to warn about missing and
unneeded header inclusion, and this is a sample set of changes that were
suggested for some of the files in base/.   I'm curious to know if these changes
work and are desirable before I submit large changes and try to get this added
to some sort of pre-integration testing.

[Nico, 2013-08-29 17:35:59]

In principle, I like this patch. In practice, IWYU has been not production-ready
every time we looked at it.

The first 2 files are arguably wrong, and if <pair> is really needed for the
iterator (maybe I missed something else) then that's definitely a bug.

https://codereview.chromium.org/23540009/diff/8001/base/time/default_clock.h
File base/time/default_clock.h (right):

https://codereview.chromium.org/23540009/diff/8001/base/time/default_clock.h#...
base/time/default_clock.h:11: #include "base/time/time.h"
Does this one make sense? Time is only used in the Now() function, which is an
OVERRIDE, so the include for the superclass necessarily pulls time.h in already,
no?

https://codereview.chromium.org/23540009/diff/8001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/23540009/diff/8001/base/values.h#newcode25
base/values.h:25: #include <utility>
What's this needed for? Is this iwyu not handling map iterators correctly?

[Mostyn Bramley-Moore, 2013-08-29 18:43:50]

> The first 2 files are arguably wrong

Here's a snippet of the IWYU output for the first file:
../../base/time/default_tick_clock.h should add these lines:
#include "base/time/time.h"             // for TimeTicks

And the second file:
../../base/time/time.h should add these lines:
#include "build/build_config.h"         // for OS_POSIX

../../base/time/time.h should remove these lines:
- #include "base/atomicops.h"  // lines 28-28

These don't look immediately unreasonable to me.

https://codereview.chromium.org/23540009/diff/8001/base/time/default_clock.h
File base/time/default_clock.h (right):

https://codereview.chromium.org/23540009/diff/8001/base/time/default_clock.h#...
base/time/default_clock.h:11: #include "base/time/time.h"
On 2013/08/29 17:35:59, Nico wrote:
> Does this one make sense? Time is only used in the Now() function, which is an
> OVERRIDE, so the include for the superclass necessarily pulls time.h in
already,
> no?

Done.

(I'll put together a list of the cases where we think IWYU reports something we
disagree with.)

https://codereview.chromium.org/23540009/diff/8001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/23540009/diff/8001/base/values.h#newcode25
base/values.h:25: #include <utility>
On 2013/08/29 17:35:59, Nico wrote:
> What's this needed for? Is this iwyu not handling map iterators correctly?

std::map<X,Y>::const_iterator ("it_") is an std::pair<X,Y> and we use some of
it's methods so I guess we need its header.  Is this considered unnecessary?

[Nico, 2013-08-29 18:50:02]

lgtm

https://codereview.chromium.org/23540009/diff/8001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/23540009/diff/8001/base/values.h#newcode25
base/values.h:25: #include <utility>
On 2013/08/29 18:43:51, Mostyn Bramley-Moore wrote:
> On 2013/08/29 17:35:59, Nico wrote:
> > What's this needed for? Is this iwyu not handling map iterators correctly?
> 
> std::map<X,Y>::const_iterator ("it_") is an std::pair<X,Y> and we use some of
> it's methods so I guess we need its header.  Is this considered unnecessary?

It feels strange to me since pair is something map uses, so the map header
should bring it in – but I suppose map is a template, so it can get away without
doing that? So I guess this one is fine.

https://codereview.chromium.org/23540009/diff/18001/base/time/default_tick_cl...
File base/time/default_tick_clock.h (right):

https://codereview.chromium.org/23540009/diff/18001/base/time/default_tick_cl...
base/time/default_tick_clock.h:21: virtual TimeTicks NowTicks() OVERRIDE;
I was thinking of the same thing here as in the previous file: Since this is
overridden from the superclass, the explicit time.h include shouldn't be needed
here

[Mostyn Bramley-Moore, 2013-08-29 19:08:34]

Should I queue this, or do you want me to go through with a fine-toothed comb? 
(For example, I could remove all the stddef.h inclusions due to the use of
NULL.)

https://codereview.chromium.org/23540009/diff/18001/base/time/default_tick_cl...
File base/time/default_tick_clock.h (right):

https://codereview.chromium.org/23540009/diff/18001/base/time/default_tick_cl...
base/time/default_tick_clock.h:21: virtual TimeTicks NowTicks() OVERRIDE;
On 2013/08/29 18:50:02, Nico wrote:
> I was thinking of the same thing here as in the previous file: Since this is
> overridden from the superclass, the explicit time.h include shouldn't be
needed
> here

Done.

Oh I see.  I'll report this as an IWYU feature request.

[Nico, 2013-08-29 19:51:53]

you can cq this

[commit-bot: I haz the power, 2013-08-29 19:54:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/23540009/21001

[commit-bot: I haz the power, 2013-08-29 20:00:38]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-08-29 20:09:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/23540009/21001

[commit-bot: I haz the power, 2013-08-29 21:09:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Mostyn Bramley-Moore, 2013-08-30 08:03:59]

@yoz: can you please review chrome/browser/extensions/blacklist.cc ?

@gene: can you please review cloud_print/service/win/cloud_print_service.cc ?

@rlarocque: and can you please review sync/notifier/p2p_invalidator.cc ?

[rlarocque, 2013-08-30 17:08:43]

https://codereview.chromium.org/23540009/diff/52001/sync/notifier/p2p_invalid...
File sync/notifier/p2p_invalidator.cc (right):

https://codereview.chromium.org/23540009/diff/52001/sync/notifier/p2p_invalid...
sync/notifier/p2p_invalidator.cc:8: #include <iterator>
I don't see why IWYU thought this was necessary.

[Mostyn Bramley-Moore, 2013-08-30 17:14:47]

https://codereview.chromium.org/23540009/diff/52001/sync/notifier/p2p_invalid...
File sync/notifier/p2p_invalidator.cc (right):

https://codereview.chromium.org/23540009/diff/52001/sync/notifier/p2p_invalid...
sync/notifier/p2p_invalidator.cc:8: #include <iterator>
On 2013/08/30 17:08:43, rlarocque wrote:
> I don't see why IWYU thought this was necessary.  

I didn't run IWYU on this file, but after making some of the suggested changes
in base/* this file failed to build with this message:

FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
"C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\bin\cl.exe" /nologo
/showIncludes /FC
@obj\chrome\browser\extensions\browser_extensions.blacklist.obj.rsp /c
..\..\chrome\browser\extensions\blacklist.cc
/Foobj\chrome\browser\extensions\browser_extensions.blacklist.obj
/Fdobj\chrome\browser_extensions.pdb 
e:\b\build\slave\win\build\src\base\stl_util.h(216) : error C2039: 'inserter' :
is not a member of 'std'
       
e:\b\build\slave\win\build\src\chrome\browser\extensions\blacklist.cc(223) : see
reference to function template instantiation 'ResultType
base::STLSetDifference<std::set<_Kty>,std::set<_Kty>,std::set<_Kty>>(const Arg1
&,const Arg2 &)' being compiled
        with
        [
            ResultType=std::set<std::string>,
            _Kty=std::string,
            Arg1=std::set<std::string>,
            Arg2=std::set<std::string>
        ]
e:\b\build\slave\win\build\src\base\stl_util.h(216) : error C3861: 'inserter':
identifier not found

So it's the use of std::inserter that requires <iterator>.

[rlarocque, 2013-08-30 17:32:59]

On 2013/08/30 17:14:47, Mostyn Bramley-Moore wrote:
>
https://codereview.chromium.org/23540009/diff/52001/sync/notifier/p2p_invalid...
> File sync/notifier/p2p_invalidator.cc (right):
> 
>
https://codereview.chromium.org/23540009/diff/52001/sync/notifier/p2p_invalid...
> sync/notifier/p2p_invalidator.cc:8: #include <iterator>
> On 2013/08/30 17:08:43, rlarocque wrote:
> > I don't see why IWYU thought this was necessary.  
> 
> I didn't run IWYU on this file, but after making some of the suggested changes
> in base/* this file failed to build with this message:
> 
> FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
> "C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\bin\cl.exe" /nologo
> /showIncludes /FC
> @obj\chrome\browser\extensions\browser_extensions.blacklist.obj.rsp /c
> ..\..\chrome\browser\extensions\blacklist.cc
> /Foobj\chrome\browser\extensions\browser_extensions.blacklist.obj
> /Fdobj\chrome\browser_extensions.pdb 
> e:\b\build\slave\win\build\src\base\stl_util.h(216) : error C2039: 'inserter'
:
> is not a member of 'std'
>        
> e:\b\build\slave\win\build\src\chrome\browser\extensions\blacklist.cc(223) :
see
> reference to function template instantiation 'ResultType
> base::STLSetDifference<std::set<_Kty>,std::set<_Kty>,std::set<_Kty>>(const
Arg1
> &,const Arg2 &)' being compiled
>         with
>         [
>             ResultType=std::set<std::string>,
>             _Kty=std::string,
>             Arg1=std::set<std::string>,
>             Arg2=std::set<std::string>
>         ]
> e:\b\build\slave\win\build\src\base\stl_util.h(216) : error C3861: 'inserter':
> identifier not found
> 
> So it's the use of std::inserter that requires <iterator>.

Interesting.  I would have expected that <algorithm> or <set> would have pulled
in those definitions.  Well, I suppose it does no harm to add it explicitly.

LGTM.

[gene, 2013-08-30 22:12:09]

lgtm for cloud_print/service/win/cloud_print_service.cc

[Yoyo Zhou, 2013-09-03 19:25:37]

blacklist.cc LGTM

[commit-bot: I haz the power, 2013-09-03 19:26:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/23540009/52001

[commit-bot: I haz the power, 2013-09-03 23:29:14]

Change committed as 221067