IWYU fixes for url/...

IWYU fixes for url/...

BUG=259043

[Łukasz Anforowicz, 2016-01-07 00:16:16]

lukasza@chromium.org changed reviewers:
+ abarth@chromium.org

[Łukasz Anforowicz, 2016-01-07 00:16:17]

abarth@, could you please take a look?

https://codereview.chromium.org/1561693003/diff/1/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1561693003/diff/1/url/gurl.h#newcode24
url/gurl.h:24: 
This addition by the IWYU tool is healthy, but I think is unnecessary, because
the Replacements templatized class is defined in url/url_canon.h (which is
already included above).

https://codereview.chromium.org/1561693003/diff/1/url/gurl_unittest.cc
File url/gurl_unittest.cc (right):

https://codereview.chromium.org/1561693003/diff/1/url/gurl_unittest.cc#newcode10
url/gurl_unittest.cc:10: #include "gtest/gtest-message.h"
This builds fine and probably refers to
src/testing/gtest/include/gtest/gtest-message.h
(and probably not to
src/third_party/mesa/src/src/gtest/include/gtest/gtest-message.h
src/third_party/llvm/utils/unittest/googletest/include/gtest/gtest-message.h).

I am not sure if this "relative" path is fine to use here...

In iwyu.out I see that this include is needed for Message.  I guess the usage is
hidden somewhere in EXPECT_EQ and other similar macros.  Or maybe IWYU tool is
wrong here.  I don't know.

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_fileurl.cc
File url/url_canon_fileurl.cc (left):

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_fileurl.cc#ol...
url/url_canon_fileurl.cc:12: namespace url {
On line #16 below there is #ifdef WIN32 - it probably should worry me.

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_icu.cc#newcode20
url/url_canon_icu.cc:20: #include "unicode/utypes.h"
Ooops - these should be relative to Chromium root (i.e. we should fully spell
out third_party/icu/source/common/unicode/umachine.h, etc.).  Interestingly -
this is caught by presubmit checks (i.e. no DEPS allows "unicode" as a prefix of
an include).

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_internal.h
File url/url_canon_internal.h (right):

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_internal.h#ne...
url/url_canon_internal.h:27: struct Component;
Good catch by the IWYU tool - a "const Component&" is indeed used by a few
functions below and so a fwd decl is needed in absence of include of
url/third_party/mozilla/url_parse.h (which has the full declaration of the
Component struct).

OTOH, the fwd declaration inserted by IWYU is incorrectly separating a comment
(above) from the enum it applies to (below)...

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_ip.h
File url/url_canon_ip.h (right):

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_ip.h#newcode16
url/url_canon_ip.h:16: struct Component;
Ooops - the edit by IWYU separated the comment (above) from the function it
applies to (below).

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_stdstring.h
File url/url_canon_stdstring.h (right):

https://codereview.chromium.org/1561693003/diff/1/url/url_canon_stdstring.h#n...
url/url_canon_stdstring.h:22: }  // namespace base
This fwd decl is unexpected - BasicStringPiece should already come from
base/strings/string_piece.h, right?

https://codereview.chromium.org/1561693003/diff/1/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/1561693003/diff/1/url/url_util.cc#newcode9
url/url_util.cc:9: #include <debug/vector>
I have no idea why IWYU thought that changing <vector> to <debug/vector> might
be a good idea...  I was building Debug flavor FWIW...

This is what iwyu.out said:
  ../../url/url_util.cc should add these lines:
  #include <debug/vector>                         // for vector

  ../../url/url_util.cc should remove these lines:
  - #include <vector>  // lines 10-10

Maybe this is happening because I built a debug flavor of clang/iwyu tool?

https://codereview.chromium.org/1561693003/diff/1/url/url_util.cc#newcode10
url/url_util.cc:10: #include <ostream>
I don't know why <ostream> is needed.  We do use << for streaming into a DCHECK
macro below, but 1) this should be covered by including base/logging.h + 2) we
don't use any stream-related classes.

FWIW, iwyu.out says:
  #include <ostream>  // for operator<<

https://codereview.chromium.org/1561693003/diff/20001/url/url_canon_fileurl.cc
File url/url_canon_fileurl.cc (left):

https://codereview.chromium.org/1561693003/diff/20001/url/url_canon_fileurl.c...
url/url_canon_fileurl.cc:10: #include "url/url_parse_internal.h"
Oops - these 2 removed lines are actually required on Windows.

https://codereview.chromium.org/1561693003/diff/20001/url/url_canon_icu.h
File url/url_canon_icu.h (right):

https://codereview.chromium.org/1561693003/diff/20001/url/url_canon_icu.h#new...
url/url_canon_icu.h:12: #include
"third_party/icu/source/common/unicode/ucnv_err.h"
I am not 100% sure why this include is needed (and it does mean that we need to
add .h file to DEPS and have our users transitively depend-on / include ICU
headers).  I am guessing that IWYU wanted this include to get a declaration of
the UConverter struct (OTOH url_canon_icu.h does contain a typedef that I think 
was supposed to do the forward of UConverter struct)?

FWIW, iwyu.out says:
  #include "third_party/icu/source/common/unicode/ucnv_err.h"  // for UConverter

https://codereview.chromium.org/1561693003/diff/20001/url/url_canon_relative.cc
File url/url_canon_relative.cc (left):

https://codereview.chromium.org/1561693003/diff/20001/url/url_canon_relative....
url/url_canon_relative.cc:11: #include "url/url_file.h"
Oops - this removed line is actually required on Windows.

https://codereview.chromium.org/1561693003/diff/20001/url/url_util.cc
File url/url_util.cc (left):

https://codereview.chromium.org/1561693003/diff/20001/url/url_util.cc#oldcode15
url/url_util.cc:15: #include "url/url_file.h"
Oops - this removed line is actually required on Windows.
(OTOH, the next removed line seems unnecessary so far...).

https://codereview.chromium.org/1561693003/diff/40001/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/1561693003/diff/40001/url/url_util.cc#newcode20
url/url_util.cc:20: #include "url/url_parse_internal.h"
I am not sure if we care about why-comments that IWYU can generate, but omits by
default.  I assume that absence of comments is better, because it is more
consistent with the rest of Chromium style.

Example of why-comments-from-IWYU:

#include <stddef.h>                             // for NULL, size_t
#include <string.h>                             // for memcpy, strlen
#include <debug/vector>                         // for vector
#include <ostream>                              // for operator<<
#include "base/debug/leak_annotations.h"        // for ANNOTATE_LEAKING_OBJE...
#include "base/logging.h"                       // for COMPACT_GOOGLE_LOG_DC...
#include "base/strings/string_piece.h"          // for StringPiece, StringPi...
#include "base/strings/string_util.h"           // for LowerCaseEqualsASCII
#include "url/third_party/mozilla/url_parse.h"  // for Component, Parsed
#include "url/url_canon_internal.h"             // for AppendEscapedChar
#include "url/url_constants.h"                  // for kFileScheme, kFileSys...
#include "url/url_file.h"
#include "url/url_parse_internal.h"             // for CountConsecutiveSlashes

[dcheng, 2016-01-07 00:25:40]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-01-07 00:25:41]

Also, do you happen to have instructions for running this tool?

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#newcode23
url/gurl.h:23: }  // namespace url
I'm not super familiar with IWYU: I guess it also adds forward declares?

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_fileurl.cc
File url/url_canon_fileurl.cc (right):

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_fileurl.c...
url/url_canon_fileurl.cc:11: #include "url/url_file.h"  // IWYU pragma: keep
Having to do this seems like a net negative. Have you tried running it across
something like //base? How well does it deal with stuff that includes
platform-specific includes?

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_icu.cc#ne...
url/url_canon_icu.cc:7: #include <stdbool.h>
This is needed in a C++ translation unit? That's really weird... what symbol is
causing IWYU to pull this in?

[Łukasz Anforowicz, 2016-01-07 00:44:53]

On 2016/01/07 00:25:41, dcheng wrote:
> Also, do you happen to have instructions for running this tool?

Please see recent comments in crbug.com/259043

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#newcode23
url/gurl.h:23: }  // namespace url
On 2016/01/07 00:25:41, dcheng wrote:
> I'm not super familiar with IWYU: I guess it also adds forward declares?

Right.  Although in this case I am surprised by the forward declaration added
here - url/url_canon.h includes the full declaration and it is included above
(on line 16).

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_fileurl.cc
File url/url_canon_fileurl.cc (right):

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_fileurl.c...
url/url_canon_fileurl.cc:11: #include "url/url_file.h"  // IWYU pragma: keep
On 2016/01/07 00:25:41, dcheng wrote:
> Having to do this seems like a net negative. Have you tried running it across
> something like //base? 

Yes, but I haven't looked at the results.

> How well does it deal with stuff that includes
> platform-specific includes?

Probably not well :-/

Options?:
- Not use IWYU for directories with platform-specific stuff
- Manually fix mistakes and sprinkle IWYU-keep-pragmas over the code
- Build + run IWYU on Linux + Windows and somehow merge results (probably too
much effort?)

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/1561693003/diff/60001/url/url_canon_icu.cc#ne...
url/url_canon_icu.cc:7: #include <stdbool.h>
On 2016/01/07 00:25:41, dcheng wrote:
> This is needed in a C++ translation unit? That's really weird... what symbol
is
> causing IWYU to pull this in?

Errr... don't shoot the messenger, but iwyu.out says:

../../url/url_canon_icu.cc should add these lines:
#include <stdbool.h>                                         // for true, false

[Łukasz Anforowicz, 2016-01-07 00:46:33]

BTW: abarth@ + dcheng@ - please treat this CL as an experiment with IWYU tool. 
I think this CL is landable, but I am not sure if I am 100% happy with IWYU
results (and would appreciate feedback) and amount of effort to fix up IWYU
mistakes (i.e. platform-specific includes) and whether I trust IWYU (i.e. the
stddef.h for true/false is indeed weird - thanks for spotting this).

[Avi (use Gerrit), 2016-01-07 01:11:02]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2016-01-07 01:11:03]

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
File url/gurl.h (left):

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#oldcode9
url/gurl.h:9: 
:(

Can we not lose these newlines? There's a discussion on c-style about that.

[dcheng, 2016-01-07 01:20:49]

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
File url/gurl.h (left):

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#oldcode9
url/gurl.h:9: 
On 2016/01/07 at 01:11:03, Avi wrote:
> :(
> 
> Can we not lose these newlines? There's a discussion on c-style about that.

The discussion on C-style seems to indicate lean in favor of not fighting the
tools (ClangMR, IWYU). Given that the style guide does not have a hard
requirement around this, I don't see a good reason to not conform.

[Avi (use Gerrit), 2016-01-07 01:36:18]

On 2016/01/07 01:20:49, dcheng wrote:
> https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
> File url/gurl.h (left):
> 
> https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#oldcode9
> url/gurl.h:9: 
> On 2016/01/07 at 01:11:03, Avi wrote:
> > :(
> > 
> > Can we not lose these newlines? There's a discussion on c-style about that.
> 
> The discussion on C-style seems to indicate lean in favor of not fighting the
> tools (ClangMR, IWYU). Given that the style guide does not have a hard
> requirement around this, I don't see a good reason to not conform.

This is the actual google3 iwyu? Oh.

[Łukasz Anforowicz, 2016-01-07 04:45:43]

On 2016/01/07 01:36:18, Avi wrote:
> On 2016/01/07 01:20:49, dcheng wrote:
> > https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
> > File url/gurl.h (left):
> > 
> > https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#oldcode9
> > url/gurl.h:9: 
> > On 2016/01/07 at 01:11:03, Avi wrote:
> > > :(
> > > 
> > > Can we not lose these newlines? There's a discussion on c-style about
that.
> > 
> > The discussion on C-style seems to indicate lean in favor of not fighting
the
> > tools (ClangMR, IWYU). Given that the style guide does not have a hard
> > requirement around this, I don't see a good reason to not conform.
> 
> This is the actual google3 iwyu? Oh.

Define "actual" :-)  The first patchset in this CL was generated with whatever
parts of IWYU got open-sourced and made available at include-what-you-use.org.

[Nico, 2016-01-08 00:34:26]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-01-08 00:34:26]

I'm pretty sure we don't want the iwyu pragma lines.

Did you manually check that the IWYU output makes sense?

Every time we evaluated it in the past it didn't work super well.

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#newcode23
url/gurl.h:23: }  // namespace url
On 2016/01/07 00:44:53, Łukasz Anforowicz wrote:
> On 2016/01/07 00:25:41, dcheng wrote:
> > I'm not super familiar with IWYU: I guess it also adds forward declares?
> 
> Right.  Although in this case I am surprised by the forward declaration added
> here - url/url_canon.h includes the full declaration and it is included above
> (on line 16).

I think in general iwyu doesn't work super well – that's why we don't use it
much.

(We used to have a wiki page for it
https://code.google.com/p/chromium/source/browse/wiki/IncludeWhatYouUse.wiki?...
but it never was really ready for chromium so we didn't look at it much)

[Łukasz Anforowicz, 2016-01-08 17:26:41]

Hmmm... based on the feedback so far, it sounds that I should start thinking
about abandoning this CL - the experiment failed? :-)

On 2016/01/08 00:34:26, Nico wrote:
> I'm pretty sure we don't want the iwyu pragma lines.

Hmmm... ok.  If we can't have these pragmas, then IWYU is clearly a no-go, but
if we allow such pragmas, then there is some hope that IWYU will be usable for
subsets of Chromium code.
> 
> Did you manually check that the IWYU output makes sense?

I tried to:
- initial patchset in this CL is the raw IWYU output.
- subsequent patchsets are my attempt at fixing the output / make it landable.

I think IWYU didn't do super well, but did an okay job for url.  Problems:
- Windows-specific includes (could be solved by IWYU pragmas)
- Mysterious stdbool.h (but then - it doesn't hurt)
- <debug/vector> instead of <vector>
 
> Every time we evaluated it in the past it didn't work super well.
> 
Yes - it probably doesn't work well enough to use it broadly / on a large scale.
 At the same time, I think I'll try experimenting with applying IWYU to small
CLs that I am working on (hoping that vetting results on a smaller number of
files will be easier and that in my case CLs are usually platform agnostic).  I
also wonder how much trouble would it be to enable "git cl iwyu -C out/gn" -
having this would let other people experiment with IWYU on their CLs.

The problem with IWYU is that it is difficult the pain caused by missing or
extraneous headers.  Missing headers are a problem during large scale
refactorings (but the pain is only felt by the few people that tackle these
refactorings, so the average pain per Chromium developer is small).  Extraneous
headers probably increase build times but it is unclear by how much.  And there
is also the pain of having to keep paying attention to the opportunities for
manual header cleanups (i.e. see [1]), although it is not clear if the IWYU tool
would help alleviate this pain or increase this pain further.  And it is also
unclear how much IWYU tool would help with the refactoring pain (because some
problems are in third_party code which we wouldn't be fixing by IWYU).

[1]
https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...
and also
https://codereview.chromium.org/1547593002/diff/100001/chrome/browser/chromeo...

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1561693003/diff/60001/url/gurl.h#newcode23
url/gurl.h:23: }  // namespace url
On 2016/01/08 00:34:26, Nico wrote:
> On 2016/01/07 00:44:53, Łukasz Anforowicz wrote:
> > On 2016/01/07 00:25:41, dcheng wrote:
> > > I'm not super familiar with IWYU: I guess it also adds forward declares?
> > 
> > Right.  Although in this case I am surprised by the forward declaration
added
> > here - url/url_canon.h includes the full declaration and it is included
above
> > (on line 16).
> 
> I think in general iwyu doesn't work super well –&nbsp;that's why we don't use
it
> much.
> 
> (We used to have a wiki page for it
>
https://code.google.com/p/chromium/source/browse/wiki/IncludeWhatYouUse.wiki?...
> but it never was really ready for chromium so we didn't look at it much)

Thanks for the reference - I've added the link to crbug.com/259043.

[Nico, 2016-01-08 17:35:11]

https://code.google.com/p/chromium/issues/detail?id=242216 is similar too.

In the past, we had hoped that iwyu will allow us to _remove_ unnecessary
includes, which hopefully will help with build time (which should be pretty
quantifiable). New includes it adds are only for things that have been included
indirectly usually, so I'd expect these to not hurt compile time.

[dcheng, 2016-01-08 17:43:10]

On 2016/01/08 at 17:35:11, thakis wrote:
> https://code.google.com/p/chromium/issues/detail?id=242216 is similar too.
> 
> In the past, we had hoped that iwyu will allow us to _remove_ unnecessary
includes, which hopefully will help with build time (which should be pretty
quantifiable). New includes it adds are only for things that have been included
indirectly usually, so I'd expect these to not hurt compile time.

Another angle on this: if IWYU could somehow process just the standard set of
C/C++ system headers (e.g. stdio.h, string) and leave the others alone, that
would be kind of a nice improvement. I know avi@ hit all sorts of 'this file
didn't bother including stddef.h because basictypes.h did' fun. I expect
something similar to happen when we try to change scoped_ptr to a typedef.