🌱 GN: Look at all target outputs when detecting duplicates

GN: Look at all target outputs when detecting duplicates

BUG=554660
Committed: https://crrev.com/83671be906e98ac69bbfaf2c2206977806ff5448
Cr-Commit-Position: refs/heads/master@{#384357}

[agrieve, 2016-03-24 01:55:31]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2016-03-25 22:43:10]

https://codereview.chromium.org/1828113002/diff/1/tools/gn/ninja_build_writer...
File tools/gn/ninja_build_writer_unittest.cc (right):

https://codereview.chromium.org/1828113002/diff/1/tools/gn/ninja_build_writer...
tools/gn/ninja_build_writer_unittest.cc:70: EXPECT_THAT(ninja_out.str(),
testing::HasSubstr(expected_rule_gn));
I'm not a fan of gmock and haven't used it in the GN unit tests so far. I think
this would be better written as

std::string out_str = ninja_out.str();
EXPECT_NE(out_str.end(), out_str.find(expected_rule_gn));
...

<begin rant to the universe>

Your code creates a PolymorphicMatches<HasSubstrMatcher> which is a functor
factory thing that creates a MonomorphicMatcher<std::string> which forwards to a
HasSubstrMatcher<std::string> which executes find != end. This is all sort of
readable until it goes wrong and then you're totally screwed with 1000 lines of
template errors and are unable to step into anything in a debugger if it fails.

Also, while it's easy to read this gmock line like English, even expert C++
programmers that don't happen to know GMock well couldn't even describe what
this test is actually doing. My suggestion is slightly less semantically less
readable but is 100% clear to any C++ programmer and easy to debug if necessary.

[agrieve, 2016-03-29 15:59:33]

https://codereview.chromium.org/1828113002/diff/1/tools/gn/ninja_build_writer...
File tools/gn/ninja_build_writer_unittest.cc (right):

https://codereview.chromium.org/1828113002/diff/1/tools/gn/ninja_build_writer...
tools/gn/ninja_build_writer_unittest.cc:70: EXPECT_THAT(ninja_out.str(),
testing::HasSubstr(expected_rule_gn));
On 2016/03/25 22:43:10, brettw wrote:
> I'm not a fan of gmock and haven't used it in the GN unit tests so far. I
think
> this would be better written as
> 
> std::string out_str = ninja_out.str();
> EXPECT_NE(out_str.end(), out_str.find(expected_rule_gn));
> ...
> 
> <begin rant to the universe>
> 
> Your code creates a PolymorphicMatches<HasSubstrMatcher> which is a functor
> factory thing that creates a MonomorphicMatcher<std::string> which forwards to
a
> HasSubstrMatcher<std::string> which executes find != end. This is all sort of
> readable until it goes wrong and then you're totally screwed with 1000 lines
of
> template errors and are unable to step into anything in a debugger if it
fails.
> 
> Also, while it's easy to read this gmock line like English, even expert C++
> programmers that don't happen to know GMock well couldn't even describe what
> this test is actually doing. My suggestion is slightly less semantically less
> readable but is 100% clear to any C++ programmer and easy to debug if
necessary.

Thanks! Helpful rant actually. I agree!

[brettw, 2016-03-31 17:52:12]

lgtm

[agrieve, 2016-03-31 18:27:04]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-03-31 18:27:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828113002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828113002/20001

[commit-bot: I haz the power, 2016-03-31 19:33:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-31 19:35:00]

Description was changed from

==========
GN: Look at all target outputs when detecting duplicates

BUG=554660
==========

to

==========
GN: Look at all target outputs when detecting duplicates

BUG=554660

Committed: https://crrev.com/83671be906e98ac69bbfaf2c2206977806ff5448
Cr-Commit-Position: refs/heads/master@{#384357}
==========

[commit-bot: I haz the power, 2016-03-31 19:35:01]

Patchset 2 (id:??) landed as
https://crrev.com/83671be906e98ac69bbfaf2c2206977806ff5448
Cr-Commit-Position: refs/heads/master@{#384357}