Testing: Fixed printing FilePaths when a test fails comparing them.

Testing: Fixed printing FilePaths when a test fails comparing them.

Moved PrintTo (for FilePath) into the base namespace (it was already
implemented, but in the wrong namespace). The PrintTo function has to be
in the same namespace as the class being printed.

BUG=488344
Committed: https://crrev.com/abf18048b9a498ced34e2f8dff1fa1dcf6737927
Cr-Commit-Position: refs/heads/master@{#331763}

[Matt Giuca, 2015-05-15 05:06:01]

mgiuca@chromium.org changed reviewers:
+ rvargas@chromium.org

[Matt Giuca, 2015-05-21 04:52:20]

mgiuca@chromium.org changed reviewers:
+ danakj@chromium.org
- rvargas@chromium.org

[Matt Giuca, 2015-05-21 04:52:20]

Just noticed that rvargas is on extended leave. +danakj, would you be able to
review this? Cheers.

[danakj, 2015-05-22 20:59:24]

https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h
File base/files/file_path.h (right):

https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h#...
base/files/file_path.h:437: BASE_EXPORT extern void PrintTo(const FilePath&
path, std::ostream* out);
While you're here, including std::ostream everywhere isn't free, and linking
this into production code for gtest isn't quite right.

James did something nice for our gfx types to support PrintTo in tests, but not
in prod:
https://chromium.googlesource.com/chromium/src/+/a109fd05fac9af35d219de6e4285...

Can we do something like that for this? (ie use isofwd only and move the defn to
a test build target.)

[Matt Giuca, 2015-05-25 03:40:09]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[Matt Giuca, 2015-05-25 03:40:15]

https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h
File base/files/file_path.h (right):

https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h#...
base/files/file_path.h:437: BASE_EXPORT extern void PrintTo(const FilePath&
path, std::ostream* out);
Done (moved to test_support_base), but...

On 2015/05/22 20:59:23, danakj wrote:
> While you're here, including std::ostream everywhere isn't free, and linking
> this into production code for gtest isn't quite right.

I'm confused about where std::ostream is coming from. I went to remove "#include
<iostream>" but it wasn't there... does that mean one of the other header files
is including it?

This means regardless of whether I do this change, won't we still be paying the
cost of iostream? (I'm unsure exactly what the cost is... is it having to
process the iostream headers in every file that includes file_path.h? Or is it
linking in the ostream code at all?)

[commit-bot: I haz the power, 2015-05-25 03:40:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139883004/60001

[commit-bot: I haz the power, 2015-05-25 05:04:22]

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

[commit-bot: I haz the power, 2015-05-25 05:04:23]

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

[Matt Giuca, 2015-05-25 05:54:02]

https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h
File base/files/file_path.h (right):

https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h#...
base/files/file_path.h:437: BASE_EXPORT extern void PrintTo(const FilePath&
path, std::ostream* out);
Sorry, by <iostream> I meant <ostream>.

[danakj, 2015-05-26 18:40:18]

On 2015/05/25 03:40:15, Matt Giuca wrote:
> https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h
> File base/files/file_path.h (right):
> 
>
https://codereview.chromium.org/1139883004/diff/40001/base/files/file_path.h#...
> base/files/file_path.h:437: BASE_EXPORT extern void PrintTo(const FilePath&
> path, std::ostream* out);
> Done (moved to test_support_base), but...
> 
> On 2015/05/22 20:59:23, danakj wrote:
> > While you're here, including std::ostream everywhere isn't free, and linking
> > this into production code for gtest isn't quite right.
> 
> I'm confused about where std::ostream is coming from. I went to remove
"#include
> <iostream>" but it wasn't there... does that mean one of the other header
files
> is including it?
> 
> This means regardless of whether I do this change, won't we still be paying
the
> cost of iostream? (I'm unsure exactly what the cost is... is it having to
> process the iostream headers in every file that includes file_path.h? Or is it
> linking in the ostream code at all?)

Right, I saw this too and am wondering where it's coming from. Can you track it
down? You could try running the compiler so that it dumps out the preprocessed
.cc file and find it that way?

[Matt Giuca, 2015-05-27 01:27:32]

> Right, I saw this too and am wondering where it's coming from. Can you track
it
> down? You could try running the compiler so that it dumps out the preprocessed
> .cc file and find it that way?

I'm still not sure what cost you're concerned about (is it the runtime cost of
processing unnecessary headers, or the binary size? Do you care about processing
the ostream header from file_path.cc, or just from the header file?)

It looks like:

- #include <string> is pulling in <localefwd> which pulls in <iosfwd>.
- #include "base/containers/hash_tables.h" is pulling in <ext/hash_map> and
<ext/hash_map> on Linux, which in turn pulls in the entire of <ostream> (but not
<iostream>).

Both of these are included from file_path.h. So if you care about not pulling in
<ostream>, maybe hash_tables needs to be fixed (but I don't want to block this
CL on that fix; this was supposed to be a very quick CL to fix something that's
been irritating me).

[danakj, 2015-05-27 17:44:45]

Thanks for checking that out for me, it's good to understand the behaviour of
things. I agree this doesn't need to block on that. LGTM

[Matt Giuca, 2015-05-28 00:51:40]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-05-28 00:52:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139883004/80001

[commit-bot: I haz the power, 2015-05-28 02:10:36]

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

[commit-bot: I haz the power, 2015-05-28 02:10:38]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Matt Giuca, 2015-05-28 07:30:05]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-05-28 07:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1139883004/#ps100001
(title: "Remove BASE_EXPORT; see if this fixes Windows compile.")

[commit-bot: I haz the power, 2015-05-28 07:30:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139883004/100001

[commit-bot: I haz the power, 2015-05-28 10:11:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-28 10:12:44]

Patchset 6 (id:??) landed as
https://crrev.com/abf18048b9a498ced34e2f8dff1fa1dcf6737927
Cr-Commit-Position: refs/heads/master@{#331763}