Implementing erase/erase_if functions from library fundamentals ts:

Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> base::Erase
erase_if -> base::EraseIf

BUG=697235
Review-Url: https://codereview.chromium.org/2723853002
Cr-Commit-Position: refs/heads/master@{#455322}
Committed: https://chromium.googlesource.com/chromium/src/+/5f0ff79896d3a6aa7a93de31cfa192c2312e5d7f

[dyaroshev, 2017-02-28 23:23:54]

Description was changed from

==========
Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> STLErase
erase_if -> STLEraseIf

BUG=697235
==========

to

==========
Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> STLErase
erase_if -> STLEraseIf

BUG=697235
==========

[dyaroshev, 2017-02-28 23:23:54]

dyaroshev@yandex-team.ru changed reviewers:
+ brettw@chromium.org, danakj@chromium.org, pkasting@chromium.org

[dyaroshev, 2017-02-28 23:28:33]

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode140
base/stl_util.h:140: // implement this functions for arbitrary containers.
Three times the word provide.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode157
base/stl_util.h:157: // ValueType<Container> is equality comparable with Value.
@pkasting - I know you don't like describing concepts like this but I really
hope you reconsider.
This is a modern c++ style and a version of it is suggested by c++ core
guidelines.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#...

[Peter Kasting, 2017-03-01 03:20:10]

pkasting@chromium.org changed reviewers:
+ gab@chromium.org
- danakj@chromium.org

[Peter Kasting, 2017-03-01 03:20:11]

LGTM with nits.

Changing OWNER reviewer from danakj to gab due to OOO.

Other reviewers, note that I requested this be added to our STL utils; it's not
only a common, verbose, and error-prone pattern in the codebase, it's hopefully
just a stopgap equivalent to a standardized mechanism coming in future years of
the C++ standard.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode140
base/stl_util.h:140: // implement this functions for arbitrary containers.
Nit: I would make this sentence "The functions here implement these for the
standard containers until those functions are available in the C++ standard." or
similar.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode146
base/stl_util.h:146: // STLEraseRemove,STLEraseRemoveIf, STLEraseByOneIf,
STLEraseMember,
Nit: Space after comma

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode148
base/stl_util.h:148: // overload is not provided here or if you need not default
behavior like
Nit: not default -> non-default

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode151
base/stl_util.h:151: // provide STLEraseByOne either.
Nit: I think this note is unnecessary given the similar note below on the APIs
most people actually would call.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode157
base/stl_util.h:157: // ValueType<Container> is equality comparable with Value.
On 2017/02/28 23:28:33, dyaroshev wrote:
> @pkasting - I know you don't like describing concepts like this but I really
> hope you reconsider.
> This is a modern c++ style and a version of it is suggested by c++ core
> guidelines.
>
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#...

With respect to the folks writing the C++ Core Guidelines, who are definitely
stronger coders than me, I'm still not a fan of placing these concept-style
comments in the midst of the function declaration.  I think until the time when
they're real assertions the compiler can do something with, we'd be clearer
without them.

I think if we want to recommend doing this, it's something that should be
discussed on the cxx mailing list, not introduced one-off in individual CLs.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode175
base/stl_util.h:175: // erasing by one element.
Nit: Capitalize start of comment sentences (multiple places)

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode180
base/stl_util.h:180: if (pred(*it)) {
Nit: No {}

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode213
base/stl_util.h:213: //
http://en.cppreference.com/w/cpp/experimental/basic_string/erase
Nit: I might (not sure) remove all these comments and possibly many or all of
the blank lines between the functions as well.  Basically just trying to reduce
boilerplate.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util_unittest.cc
File base/stl_util_unittest.cc (right):

https://codereview.chromium.org/2723853002/diff/1/base/stl_util_unittest.cc#n...
base/stl_util_unittest.cc:65: {{PairFromInt(1), PairFromInt(2), PairFromInt(3)},
Nit: This all seems longer than just writing {1, 1}, {2, 2}, {3, 3}.

[danakj, 2017-03-01 18:58:44]

On 2017/03/01 03:20:11, Peter Kasting wrote:
> LGTM with nits.
> 
> Changing OWNER reviewer from danakj to gab due to OOO.

Hey, I'm back today and happy to have a look.

> Other reviewers, note that I requested this be added to our STL utils; it's
not
> only a common,

I found 70 uses of std::remove_if() in all of chrome:
https://cs.chromium.org/search/?q=std::remove_if+-f:src/third_party+-f:src/v8...

> verbose,

I agree that writing foo.begin(), foo.end() is a bit verbose.

> and error-prone

But I don't understand what is error-prone. Do you have examples?

> pattern in the codebase, it's hopefully
> just a stopgap equivalent to a standardized mechanism coming in future years
of
> the C++ standard.

I see this is in the library extensions. As someone unfamiliar with the process,
what does that mean wrt it being adopted into the standard?

Also using std::remove() can produce worse code than using the container to do
searching. For instance with flat_set here:

base::STLEraseRemove(set, item) will be an O(N) operation.
set.erase(set.find(item)), or set.erase(item) is an O(log N) operation.

Using base::STLErase (or std::erase) seems like generally a bad idea..

erase_if() saves some typing, but for something an average chrome developer will
write less than 1 times in their career, I'm not convinced this is worth it.

auto pred = [](int i) { return i < 10; }

// Sure this is definitely shorter.
base::STLEraseIf(set, pred);

// But everyone knows what this means.
auto remove = std::remove_if(set.begin(), set.end(), pred);
set.erase(remove, set.end());

We're adding 360 lines of library code to avoid 70 uses of the latter?

> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h
> File base/stl_util.h (right):
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode140
> base/stl_util.h:140: // implement this functions for arbitrary containers.
> Nit: I would make this sentence "The functions here implement these for the
> standard containers until those functions are available in the C++ standard."
or
> similar.
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode146
> base/stl_util.h:146: // STLEraseRemove,STLEraseRemoveIf, STLEraseByOneIf,
> STLEraseMember,
> Nit: Space after comma
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode148
> base/stl_util.h:148: // overload is not provided here or if you need not
default
> behavior like
> Nit: not default -> non-default
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode151
> base/stl_util.h:151: // provide STLEraseByOne either.
> Nit: I think this note is unnecessary given the similar note below on the APIs
> most people actually would call.
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode157
> base/stl_util.h:157: // ValueType<Container> is equality comparable with
Value.
> On 2017/02/28 23:28:33, dyaroshev wrote:
> > @pkasting - I know you don't like describing concepts like this but I really
> > hope you reconsider.
> > This is a modern c++ style and a version of it is suggested by c++ core
> > guidelines.
> >
>
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#...
> 
> With respect to the folks writing the C++ Core Guidelines, who are definitely
> stronger coders than me, I'm still not a fan of placing these concept-style
> comments in the midst of the function declaration.  I think until the time
when
> they're real assertions the compiler can do something with, we'd be clearer
> without them.
> 
> I think if we want to recommend doing this, it's something that should be
> discussed on the cxx mailing list, not introduced one-off in individual CLs.
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode175
> base/stl_util.h:175: // erasing by one element.
> Nit: Capitalize start of comment sentences (multiple places)
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode180
> base/stl_util.h:180: if (pred(*it)) {
> Nit: No {}
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode213
> base/stl_util.h:213: //
> http://en.cppreference.com/w/cpp/experimental/basic_string/erase
> Nit: I might (not sure) remove all these comments and possibly many or all of
> the blank lines between the functions as well.  Basically just trying to
reduce
> boilerplate.
> 
> https://codereview.chromium.org/2723853002/diff/1/base/stl_util_unittest.cc
> File base/stl_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/2723853002/diff/1/base/stl_util_unittest.cc#n...
> base/stl_util_unittest.cc:65: {{PairFromInt(1), PairFromInt(2),
PairFromInt(3)},
> Nit: This all seems longer than just writing {1, 1}, {2, 2}, {3, 3}.

[danakj, 2017-03-01 18:58:54]

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

[dyaroshev, 2017-03-01 19:26:22]

On 2017/03/01 18:58:44, danakj wrote:
> On 2017/03/01 03:20:11, Peter Kasting wrote:
> > LGTM with nits.
> > 
> > Changing OWNER reviewer from danakj to gab due to OOO.
> 
> Hey, I'm back today and happy to have a look.
>

Great!
 
> > Other reviewers, note that I requested this be added to our STL utils; it's
> not
> > only a common,
> 
> I found 70 uses of std::remove_if() in all of chrome:
>
https://cs.chromium.org/search/?q=std::remove_if+-f:src/third_party+-f:src/v8...
>
> > verbose,
> 
> I agree that writing foo.begin(), foo.end() is a bit verbose.
> 
> > and error-prone
> 
> But I don't understand what is error-prone. Do you have examples?
> 

1) I'm constantly forgetting end in erase and I've seen other people do this.
Like vec.erase(std::remove_if(vec.begin(), vec.end()); This is an unpleasant bug
because it often leads to undefined behaviour. 
If your test coverage is not sufficient, it can blow up in your face.

2) For sets/maps it's a messy loop with iterators, I've made a few errors
recently.
3) When switching from std::set to flat_set - erases are better to be fixed, and
if they are written like this:
(A recommended way from Scott Meyers book)
    for (WordIDSet::iterator word_set_iter = word_id_set.begin();
         word_set_iter != word_id_set.end(); ) {
      if (word_list_[*word_set_iter].find(term) == base::string16::npos)
         word_id_set.erase(word_set_iter++);
      else
        ++word_set_iter;
    }

It will blow up badly.

> > pattern in the codebase, it's hopefully
> > just a stopgap equivalent to a standardized mechanism coming in future years
> of
> > the C++ standard.
> 
> I see this is in the library extensions. As someone unfamiliar with the
process,
> what does that mean wrt it being adopted into the standard?
> 

TS (like library fundamentals TS) is essentially a beta version of some parts of
the standard. The standardisation committee thinks it's good but they want field
results before putting it in the standards.

> Also using std::remove() can produce worse code than using the container to do
> searching. For instance with flat_set here:
> 
> base::STLEraseRemove(set, item) will be an O(N) operation.
> set.erase(set.find(item)), or set.erase(item) is an O(log N) operation.
> 
> Using base::STLErase (or std::erase) seems like generally a bad idea..

a) There is no std::erase for associative containers (including flat_sets), so
this case won't compile. There are some good reasons for it, like it would have
different requirements for type that we compare against.
b) If there was, it should call an erase method on the container.

> 
> erase_if() saves some typing, but for something an average chrome developer
will
> write less than 1 times in their career, I'm not convinced this is worth it.
> 
> auto pred = [](int i) { return i < 10; }
> 
> // Sure this is definitely shorter.
> base::STLEraseIf(set, pred);
> 
> // But everyone knows what this means.
> auto remove = std::remove_if(set.begin(), set.end(), pred);
> set.erase(remove, set.end());
> 
> We're adding 360 lines of library code to avoid 70 uses of the latter?
> 

It's not only remove_if, you can look at example like this one: 
https://cs.chromium.org/chromium/src/components/omnibox/browser/url_index_pri...

And some times people do this:
https://cs.chromium.org/chromium/src/components/omnibox/browser/keyword_provi...

Maybe the library will help.

[dyaroshev, 2017-03-01 20:05:37]

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode140
base/stl_util.h:140: // implement this functions for arbitrary containers.
On 2017/03/01 03:20:10, Peter Kasting wrote:
> Nit: I would make this sentence "The functions here implement these for the
> standard containers until those functions are available in the C++ standard."
or
> similar.

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode146
base/stl_util.h:146: // STLEraseRemove,STLEraseRemoveIf, STLEraseByOneIf,
STLEraseMember,
On 2017/03/01 03:20:11, Peter Kasting wrote:
> Nit: Space after comma

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode148
base/stl_util.h:148: // overload is not provided here or if you need not default
behavior like
On 2017/03/01 03:20:10, Peter Kasting wrote:
> Nit: not default -> non-default

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode151
base/stl_util.h:151: // provide STLEraseByOne either.
On 2017/03/01 03:20:11, Peter Kasting wrote:
> Nit: I think this note is unnecessary given the similar note below on the APIs
> most people actually would call.

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode157
base/stl_util.h:157: // ValueType<Container> is equality comparable with Value.
On 2017/03/01 03:20:11, Peter Kasting wrote:
> On 2017/02/28 23:28:33, dyaroshev wrote:
> > @pkasting - I know you don't like describing concepts like this but I really
> > hope you reconsider.
> > This is a modern c++ style and a version of it is suggested by c++ core
> > guidelines.
> >
>
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#...
> 
> With respect to the folks writing the C++ Core Guidelines, who are definitely
> stronger coders than me, I'm still not a fan of placing these concept-style
> comments in the midst of the function declaration.  I think until the time
when
> they're real assertions the compiler can do something with, we'd be clearer
> without them.
> 
> I think if we want to recommend doing this, it's something that should be
> discussed on the cxx mailing list, not introduced one-off in individual CLs.

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode175
base/stl_util.h:175: // erasing by one element.
On 2017/03/01 03:20:10, Peter Kasting wrote:
> Nit: Capitalize start of comment sentences (multiple places)

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode180
base/stl_util.h:180: if (pred(*it)) {
On 2017/03/01 03:20:11, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/2723853002/diff/1/base/stl_util.h#newcode213
base/stl_util.h:213: //
http://en.cppreference.com/w/cpp/experimental/basic_string/erase
On 2017/03/01 03:20:11, Peter Kasting wrote:
> Nit: I might (not sure) remove all these comments and possibly many or all of
> the blank lines between the functions as well.  Basically just trying to
reduce
> boilerplate.

I removed the comments. I don't know about the blank lines. Maybe create a
separate header and include it here?

[Peter Kasting, 2017-03-01 22:58:19]

dyaroshev's answers are good for the most part, I'll just add a bit.

On 2017/03/01 18:58:44, danakj wrote:
> I found 70 uses of std::remove_if() in all of chrome:
>
https://cs.chromium.org/search/?q=std::remove_if+-f:src/third_party+-f:src/v8...

Yes; this also somewhat undercounts the usage, because for various reasons
(verbosity, unfamiliarity with <algorithm>) people don't use this idiom in all
places where it might make sense to do so.  Trying to make it more painless and
bulletproof to do so will hopefully help promote it.

I still wouldn't necessarily have suggested adding this to base/stl_util.h had
it not been part of library fundamentals TS 2.  But since it is, I view this as
basically pulling in part of the future standard early.

> > verbose,
> 
> I agree that writing foo.begin(), foo.end() is a bit verbose.

...and repeating foo.end() a second time, and making sure to nest your
erase(remove)) calls.  Which leads to...

> > and error-prone
> 
> But I don't understand what is error-prone. Do you have examples?

The one the compiler doesn't catch, that I've personally screwed up, is writing
remove_if() without writing erase() around/after it.

> Also using std::remove() can produce worse code than using the container to do
> searching. For instance with flat_set here:
> 
> base::STLEraseRemove(set, item) will be an O(N) operation.
> set.erase(set.find(item)), or set.erase(item) is an O(log N) operation.
> 
> Using base::STLErase (or std::erase) seems like generally a bad idea..

As noted, we don't actually provide this for flat_set.  In a case like
STLEraseRemoveIf(base::flat_set) that we do provide, this implementation is
O(N), and the "optimal" implementation is also O(N).  We shouldn't be providing
any wrappers that generate suboptimal code; if we are, please do point them out
so they can be fixed.

[dyaroshev, 2017-03-01 23:15:09]

I have a suggestion but I'm not sure you'll like it.
If we follow the standard carefully than we'll see that the predicate in
remove_if meets Predicate:
http://en.cppreference.com/w/cpp/concept/Predicate
which meens that it does not modify values.

In practice it does not seem to cause a problem if we, for example, mutate the
value we work with, but this does not match the formal specification. We can
drop this requirement from STLEraseIf spec. It won't be an exact match for
library fundamentals ts than, but it's useful.

[Peter Kasting, 2017-03-01 23:19:40]

On 2017/03/01 23:15:09, dyaroshev wrote:
> I have a suggestion but I'm not sure you'll like it.
> If we follow the standard carefully than we'll see that the predicate in
> remove_if meets Predicate:
> http://en.cppreference.com/w/cpp/concept/Predicate
> which meens that it does not modify values.
> 
> In practice it does not seem to cause a problem if we, for example, mutate the
> value we work with, but this does not match the formal specification. We can
> drop this requirement from STLEraseIf spec. It won't be an exact match for
> library fundamentals ts than, but it's useful.

I'm not sure what problem you're suggesting addressing with this suggestion.

[dyaroshev, 2017-03-02 10:31:53]

On 2017/03/01 23:19:40, Peter Kasting wrote:
> On 2017/03/01 23:15:09, dyaroshev wrote:
> > I have a suggestion but I'm not sure you'll like it.
> > If we follow the standard carefully than we'll see that the predicate in
> > remove_if meets Predicate:
> > http://en.cppreference.com/w/cpp/concept/Predicate
> > which meens that it does not modify values.
> > 
> > In practice it does not seem to cause a problem if we, for example, mutate
the
> > value we work with, but this does not match the formal specification. We can
> > drop this requirement from STLEraseIf spec. It won't be an exact match for
> > library fundamentals ts than, but it's useful.
> 
> I'm not sure what problem you're suggesting addressing with this suggestion.

This is not guarantied to work.

std::vector<int> ints = ...;
// transforming into remainders of 5 and removing all divisible by 5.
ints.erase_if(std::remove_if(ints.begin(), ints.end(), [](int& i) { return i%=
5; }), ints.end());


I haven't found an exact match for it in code but these are somewhat
questionable:
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...

We can provide a one pass guarantee for STLEraseIf

[Peter Kasting, 2017-03-02 20:12:53]

On 2017/03/02 10:31:53, dyaroshev wrote:
> On 2017/03/01 23:19:40, Peter Kasting wrote:
> > On 2017/03/01 23:15:09, dyaroshev wrote:
> > > I have a suggestion but I'm not sure you'll like it.
> > > If we follow the standard carefully than we'll see that the predicate in
> > > remove_if meets Predicate:
> > > http://en.cppreference.com/w/cpp/concept/Predicate
> > > which meens that it does not modify values.
> > > 
> > > In practice it does not seem to cause a problem if we, for example, mutate
> the
> > > value we work with, but this does not match the formal specification. We
can
> > > drop this requirement from STLEraseIf spec. It won't be an exact match for
> > > library fundamentals ts than, but it's useful.
> > 
> > I'm not sure what problem you're suggesting addressing with this suggestion.
> 
> This is not guarantied to work.
> 
> std::vector<int> ints = ...;
> // transforming into remainders of 5 and removing all divisible by 5.
> ints.erase_if(std::remove_if(ints.begin(), ints.end(), [](int& i) { return i%=
> 5; }), ints.end());
> 
> 
> I haven't found an exact match for it in code but these are somewhat
> questionable:
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> 
> We can provide a one pass guarantee for STLEraseIf

I'm still not sure what you're saying.  You're suggesting we explicitly allow
modifying the container elements within the predicate, even though the standard
doesn't?  What practical code change are you looking to make, and why would this
be beneficial?  Your links above both point to the same spot, but it looks like
the code you linked to is trying to partition out elements of one container, so
I'd tend to follow the first suggestion at
http://stackoverflow.com/questions/32155810/move-all-elements-which-satisfy-s...
to rewrite this code.

[dyaroshev, 2017-03-02 21:36:40]

On 2017/03/02 20:12:53, Peter Kasting wrote:
> On 2017/03/02 10:31:53, dyaroshev wrote:
> > On 2017/03/01 23:19:40, Peter Kasting wrote:
> > > On 2017/03/01 23:15:09, dyaroshev wrote:
> > > > I have a suggestion but I'm not sure you'll like it.
> > > > If we follow the standard carefully than we'll see that the predicate in
> > > > remove_if meets Predicate:
> > > > http://en.cppreference.com/w/cpp/concept/Predicate
> > > > which meens that it does not modify values.
> > > > 
> > > > In practice it does not seem to cause a problem if we, for example,
mutate
> > the
> > > > value we work with, but this does not match the formal specification. We
> can
> > > > drop this requirement from STLEraseIf spec. It won't be an exact match
for
> > > > library fundamentals ts than, but it's useful.
> > > 
> > > I'm not sure what problem you're suggesting addressing with this
suggestion.
> > 
> > This is not guarantied to work.
> > 
> > std::vector<int> ints = ...;
> > // transforming into remainders of 5 and removing all divisible by 5.
> > ints.erase_if(std::remove_if(ints.begin(), ints.end(), [](int& i) { return
i%=
> > 5; }), ints.end());
> > 
> > 
> > I haven't found an exact match for it in code but these are somewhat
> > questionable:
> >
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> >
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> > 
> > We can provide a one pass guarantee for STLEraseIf
> 
> I'm still not sure what you're saying.  You're suggesting we explicitly allow
> modifying the container elements within the predicate, even though the
standard
> doesn't?  What practical code change are you looking to make, and why would
this
> be beneficial?  Your links above both point to the same spot, but it looks
like
> the code you linked to is trying to partition out elements of one container,
so
> I'd tend to follow the first suggestion at
>
http://stackoverflow.com/questions/32155810/move-all-elements-which-satisfy-s...
> to rewrite this code.

Sure, stable partition would work, but it's a more complex algorithm - 
- it has to physically move elements around many times.
- it most likely (at the very least how I would write it) does recursive calls.

Remove doesn't have to do that. I suggest that we can rewrite the algorithm
without using standard remove if (it will still be the same code from
http://ru.cppreference.com/w/cpp/algorithm/remove) and drop this requirement.

It's just a suggestion.

[Peter Kasting, 2017-03-02 21:51:46]

On 2017/03/02 21:36:40, dyaroshev wrote:
> On 2017/03/02 20:12:53, Peter Kasting wrote:
> > On 2017/03/02 10:31:53, dyaroshev wrote:
> > > On 2017/03/01 23:19:40, Peter Kasting wrote:
> > > > On 2017/03/01 23:15:09, dyaroshev wrote:
> > > > > I have a suggestion but I'm not sure you'll like it.
> > > > > If we follow the standard carefully than we'll see that the predicate
in
> > > > > remove_if meets Predicate:
> > > > > http://en.cppreference.com/w/cpp/concept/Predicate
> > > > > which meens that it does not modify values.
> > > > > 
> > > > > In practice it does not seem to cause a problem if we, for example,
> mutate
> > > the
> > > > > value we work with, but this does not match the formal specification.
We
> > can
> > > > > drop this requirement from STLEraseIf spec. It won't be an exact match
> for
> > > > > library fundamentals ts than, but it's useful.
> > > > 
> > > > I'm not sure what problem you're suggesting addressing with this
> suggestion.
> > > 
> > > This is not guarantied to work.
> > > 
> > > std::vector<int> ints = ...;
> > > // transforming into remainders of 5 and removing all divisible by 5.
> > > ints.erase_if(std::remove_if(ints.begin(), ints.end(), [](int& i) { return
> i%=
> > > 5; }), ints.end());
> > > 
> > > 
> > > I haven't found an exact match for it in code but these are somewhat
> > > questionable:
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> > > 
> > > We can provide a one pass guarantee for STLEraseIf
> > 
> > I'm still not sure what you're saying.  You're suggesting we explicitly
allow
> > modifying the container elements within the predicate, even though the
> standard
> > doesn't?  What practical code change are you looking to make, and why would
> this
> > be beneficial?  Your links above both point to the same spot, but it looks
> like
> > the code you linked to is trying to partition out elements of one container,
> so
> > I'd tend to follow the first suggestion at
> >
>
http://stackoverflow.com/questions/32155810/move-all-elements-which-satisfy-s...
> > to rewrite this code.
> 
> Sure, stable partition would work, but it's a more complex algorithm - 
> - it has to physically move elements around many times.
> - it most likely (at the very least how I would write it) does recursive
calls.
> 
> Remove doesn't have to do that. I suggest that we can rewrite the algorithm
> without using standard remove if (it will still be the same code from
> http://ru.cppreference.com/w/cpp/algorithm/remove) and drop this requirement.
> 
> It's just a suggestion.

I'm still not really sure what you're proposing here, in this CL.  But it seems
like your motivation is to try to handle cases where callers are not just
removing elements from a container.

This seems like a distraction.  Let's worry about solving the problem of
conditionally erasing elements from a container and matching the upcoming
standard, and not worry about solving other outlier cases for now.  If we agree
to put this core helper in, then later we can debate whether to extend it in
some way, and have a diff in front of us to discuss practically.

[dyaroshev, 2017-03-02 22:23:59]

On 2017/03/02 21:51:46, Peter Kasting wrote:
> On 2017/03/02 21:36:40, dyaroshev wrote:
> > On 2017/03/02 20:12:53, Peter Kasting wrote:
> > > On 2017/03/02 10:31:53, dyaroshev wrote:
> > > > On 2017/03/01 23:19:40, Peter Kasting wrote:
> > > > > On 2017/03/01 23:15:09, dyaroshev wrote:
> > > > > > I have a suggestion but I'm not sure you'll like it.
> > > > > > If we follow the standard carefully than we'll see that the
predicate
> in
> > > > > > remove_if meets Predicate:
> > > > > > http://en.cppreference.com/w/cpp/concept/Predicate
> > > > > > which meens that it does not modify values.
> > > > > > 
> > > > > > In practice it does not seem to cause a problem if we, for example,
> > mutate
> > > > the
> > > > > > value we work with, but this does not match the formal
specification.
> We
> > > can
> > > > > > drop this requirement from STLEraseIf spec. It won't be an exact
match
> > for
> > > > > > library fundamentals ts than, but it's useful.
> > > > > 
> > > > > I'm not sure what problem you're suggesting addressing with this
> > suggestion.
> > > > 
> > > > This is not guarantied to work.
> > > > 
> > > > std::vector<int> ints = ...;
> > > > // transforming into remainders of 5 and removing all divisible by 5.
> > > > ints.erase_if(std::remove_if(ints.begin(), ints.end(), [](int& i) {
return
> > i%=
> > > > 5; }), ints.end());
> > > > 
> > > > 
> > > > I haven't found an exact match for it in code but these are somewhat
> > > > questionable:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?q=std::remo...
> > > > 
> > > > We can provide a one pass guarantee for STLEraseIf
> > > 
> > > I'm still not sure what you're saying.  You're suggesting we explicitly
> allow
> > > modifying the container elements within the predicate, even though the
> > standard
> > > doesn't?  What practical code change are you looking to make, and why
would
> > this
> > > be beneficial?  Your links above both point to the same spot, but it looks
> > like
> > > the code you linked to is trying to partition out elements of one
container,
> > so
> > > I'd tend to follow the first suggestion at
> > >
> >
>
http://stackoverflow.com/questions/32155810/move-all-elements-which-satisfy-s...
> > > to rewrite this code.
> > 
> > Sure, stable partition would work, but it's a more complex algorithm - 
> > - it has to physically move elements around many times.
> > - it most likely (at the very least how I would write it) does recursive
> calls.
> > 
> > Remove doesn't have to do that. I suggest that we can rewrite the algorithm
> > without using standard remove if (it will still be the same code from
> > http://ru.cppreference.com/w/cpp/algorithm/remove) and drop this
requirement.
> > 
> > It's just a suggestion.
> 
> I'm still not really sure what you're proposing here, in this CL.  But it
seems
> like your motivation is to try to handle cases where callers are not just
> removing elements from a container.
> 
> This seems like a distraction.  Let's worry about solving the problem of
> conditionally erasing elements from a container and matching the upcoming
> standard, and not worry about solving other outlier cases for now.  If we
agree
> to put this core helper in, then later we can debate whether to extend it in
> some way, and have a diff in front of us to discuss practically.

Ok.

[danakj, 2017-03-03 19:55:06]

On Thu, Mar 2, 2017 at 4:51 PM, <pkasting@chromium.org> wrote:

> On 2017/03/02 21:36:40, dyaroshev wrote:
> > On 2017/03/02 20:12:53, Peter Kasting wrote:
> > > On 2017/03/02 10:31:53, dyaroshev wrote:
> > > > On 2017/03/01 23:19:40, Peter Kasting wrote:
> > > > > On 2017/03/01 23:15:09, dyaroshev wrote:
> > > > > > I have a suggestion but I'm not sure you'll like it.
> > > > > > If we follow the standard carefully than we'll see that the
> predicate
> in
> > > > > > remove_if meets Predicate:
> > > > > > http://en.cppreference.com/w/cpp/concept/Predicate
> > > > > > which meens that it does not modify values.
> > > > > >
> > > > > > In practice it does not seem to cause a problem if we, for
> example,
> > mutate
> > > > the
> > > > > > value we work with, but this does not match the formal
> specification.
> We
> > > can
> > > > > > drop this requirement from STLEraseIf spec. It won't be an exact
> match
> > for
> > > > > > library fundamentals ts than, but it's useful.
> > > > >
> > > > > I'm not sure what problem you're suggesting addressing with this
> > suggestion.
> > > >
> > > > This is not guarantied to work.
> > > >
> > > > std::vector<int> ints = ...;
> > > > // transforming into remainders of 5 and removing all divisible by 5.
> > > > ints.erase_if(std::remove_if(ints.begin(), ints.end(), [](int& i) {
> return
> > i%=
> > > > 5; }), ints.end());
> > > >
> > > >
> > > > I haven't found an exact match for it in code but these are somewhat
> > > > questionable:
> > > >
> > >
> >
> https://cs.chromium.org/chromium/src/ui/display/win/
> screen_win.cc?q=std::remove_if+package:%5Echromium$&dr=C&l=68
> > > >
> > >
> >
> https://cs.chromium.org/chromium/src/ui/display/win/
> screen_win.cc?q=std::remove_if+package:%5Echromium$&dr=C&l=68
> > > >
> > > > We can provide a one pass guarantee for STLEraseIf
> > >
> > > I'm still not sure what you're saying. You're suggesting we explicitly
> allow
> > > modifying the container elements within the predicate, even though the
> > standard
> > > doesn't? What practical code change are you looking to make, and why
> would
> > this
> > > be beneficial? Your links above both point to the same spot, but it
> looks
> > like
> > > the code you linked to is trying to partition out elements of one
> container,
> > so
> > > I'd tend to follow the first suggestion at
> > >
> >
> http://stackoverflow.com/questions/32155810/move-all-
> elements-which-satisfy-some-condition-from-one-container-to-another-i
> > > to rewrite this code.
> >
> > Sure, stable partition would work, but it's a more complex algorithm -
> > - it has to physically move elements around many times.
> > - it most likely (at the very least how I would write it) does recursive
> calls.
> >
> > Remove doesn't have to do that. I suggest that we can rewrite the
> algorithm
> > without using standard remove if (it will still be the same code from
> > http://ru.cppreference.com/w/cpp/algorithm/remove) and drop this
> requirement.
> >
> > It's just a suggestion.
>
> I'm still not really sure what you're proposing here, in this CL. But it
> seems
> like your motivation is to try to handle cases where callers are not just
> removing elements from a container.
>
> This seems like a distraction. Let's worry about solving the problem of
> conditionally erasing elements from a container and matching the upcoming
> standard, and not worry about solving other outlier cases for now. If we
> agree
> to put this core helper in, then later we can debate whether to extend it
> in
> some way, and have a diff in front of us to discuss practically.
>

yeh, the argument for adding this is that it's going to be standardized at
which point we can remove it from base. If this is different, then we have
to go back and do a bunch of work to remove the base one.


>
> https://codereview.chromium.org/2723853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-03 20:11:49]

https://codereview.chromium.org/2723853002/diff/20001/base/containers/flat_set.h
File base/containers/flat_set.h (right):

https://codereview.chromium.org/2723853002/diff/20001/base/containers/flat_se...
base/containers/flat_set.h:665: template <typename Key, typename Compare,
typename Predicate>
Please describe the complexity of this method, there are no standard docs to
lean on here.

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode153
base/stl_util.h:153: void STLEraseRemove(Container& container, const Value&
value) {
These should be in the base::internal namespace, but TBH I'm not clear why we're
making these helper functions. It seems like we're trying to avoid writing
erase(remove()) or erase(remove_if()) at all costs, but I don't see why we
wouldn't just call that directly as needed. It would be a lot easier to read
things if base::Erase() just did what it did, rather than calling an obscurely
named function that does the work.

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode169
base/stl_util.h:169: void STLEraseByOneIf(Container& container, Predicate pred)
{
This maybe justifies a helper method in the internal namespace. How about
IterateAndEraseIf?

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode181
base/stl_util.h:181: container.remove_if(pred);
can't see why the caller wouldn't just call container.remove_if() here

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode200
base/stl_util.h:200: void STLErase(std::basic_string<CharT, Traits, Allocator>&
container,
I'm not sure where the whole STL prefix thing started, but I think these should
just be base::Erase and base::EraseIf (like base::MakeUnique, for example, or
base::is_copy_assignable etc..)

[danakj, 2017-03-03 20:12:55]

I think I can see the usefulness, thanks for your comments. I'm not totally sure
how we're going to deal with things once the standard one happens tho. We can't
add a std::erase_if() for our base::flat_set. It'll be a bit odd to have
base::EraseIf() and std::erase_if() maybe, thoughts?

[dyaroshev, 2017-03-03 21:28:33]

On 2017/03/03 20:12:55, danakj wrote:
> I think I can see the usefulness, thanks for your comments. I'm not totally
sure
> how we're going to deal with things once the standard one happens tho. We
can't
> add a std::erase_if() for our base::flat_set. It'll be a bit odd to have
> base::EraseIf() and std::erase_if() maybe, thoughts?

Well the standard suggests that erase_if is supplied in the header of a
container. I think that they mean for it to be used with ADL, so just call:
erase_if(container, predicate), not stl::erase_if. With this in mind we can
supply erase_if in base, in flat_set.h and it will work.

[danakj, 2017-03-03 21:30:46]

On Fri, Mar 3, 2017 at 4:28 PM, <dyaroshev@yandex-team.ru> wrote:

> On 2017/03/03 20:12:55, danakj wrote:
> > I think I can see the usefulness, thanks for your comments. I'm not
> totally
> sure
> > how we're going to deal with things once the standard one happens tho. We
> can't
> > add a std::erase_if() for our base::flat_set. It'll be a bit odd to have
> > base::EraseIf() and std::erase_if() maybe, thoughts?
>
> Well the standard suggests that erase_if is supplied in the header of a
> container. I think that they mean for it to be used with ADL, so just call:
> erase_if(container, predicate), not stl::erase_if. With this in mind we can
> supply erase_if in base, in flat_set.h and it will work.
>

Ok that sounds good. base::EraseIf for now, and we can change to
base::erase_if when there is std::erase_if.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dyaroshev, 2017-03-03 21:33:30]

On 2017/03/03 21:30:46, danakj wrote:
> On Fri, Mar 3, 2017 at 4:28 PM, <mailto:dyaroshev@yandex-team.ru> wrote:
> 
> > On 2017/03/03 20:12:55, danakj wrote:
> > > I think I can see the usefulness, thanks for your comments. I'm not
> > totally
> > sure
> > > how we're going to deal with things once the standard one happens tho. We
> > can't
> > > add a std::erase_if() for our base::flat_set. It'll be a bit odd to have
> > > base::EraseIf() and std::erase_if() maybe, thoughts?
> >
> > Well the standard suggests that erase_if is supplied in the header of a
> > container. I think that they mean for it to be used with ADL, so just call:
> > erase_if(container, predicate), not stl::erase_if. With this in mind we can
> > supply erase_if in base, in flat_set.h and it will work.
> >
> 
> Ok that sounds good. base::EraseIf for now, and we can change to
> base::erase_if when there is std::erase_if.
> 

How much do you mind if it will be base::erase_if now? We use hacker_case for a
few things in base.

[danakj, 2017-03-03 21:39:06]

On Fri, Mar 3, 2017 at 4:33 PM, <dyaroshev@yandex-team.ru> wrote:

> On 2017/03/03 21:30:46, danakj wrote:
> > On Fri, Mar 3, 2017 at 4:28 PM, <mailto:dyaroshev@yandex-team.ru> wrote:
> >
> > > On 2017/03/03 20:12:55, danakj wrote:
> > > > I think I can see the usefulness, thanks for your comments. I'm not
> > > totally
> > > sure
> > > > how we're going to deal with things once the standard one happens
> tho. We
> > > can't
> > > > add a std::erase_if() for our base::flat_set. It'll be a bit odd to
> have
> > > > base::EraseIf() and std::erase_if() maybe, thoughts?
> > >
> > > Well the standard suggests that erase_if is supplied in the header of a
> > > container. I think that they mean for it to be used with ADL, so just
> call:
> > > erase_if(container, predicate), not stl::erase_if. With this in mind
> we can
> > > supply erase_if in base, in flat_set.h and it will work.
> > >
> >
> > Ok that sounds good. base::EraseIf for now, and we can change to
> > base::erase_if when there is std::erase_if.
> >
>
> How much do you mind if it will be base::erase_if now? We use hacker_case
> for a
> few things in base.
>

We will have to write everything base:: right now anyway, so we will have
to rewrite everything to use STL. We should follow our style guide unless
there's a good reason, and I think this is fairly speculative still, so I
would prefer EraseIf.


>
>
>
> https://codereview.chromium.org/2723853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dyaroshev, 2017-03-03 22:48:11]

Patchset #3 (id:40001) has been deleted

[dyaroshev, 2017-03-03 23:14:29]

Patchset #3 (id:60001) has been deleted

[dyaroshev, 2017-03-03 23:16:54]

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode153
base/stl_util.h:153: void STLEraseRemove(Container& container, const Value&
value) {
On 2017/03/03 20:11:49, danakj wrote:
> These should be in the base::internal namespace, but TBH I'm not clear why
we're
> making these helper functions. It seems like we're trying to avoid writing
> erase(remove()) or erase(remove_if()) at all costs, but I don't see why we
> wouldn't just call that directly as needed. It would be a lot easier to read
> things if base::Erase() just did what it did, rather than calling an obscurely
> named function that does the work.

Done.

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode169
base/stl_util.h:169: void STLEraseByOneIf(Container& container, Predicate pred)
{
On 2017/03/03 20:11:49, danakj wrote:
> This maybe justifies a helper method in the internal namespace. How about
> IterateAndEraseIf?

Done.

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode181
base/stl_util.h:181: container.remove_if(pred);
On 2017/03/03 20:11:49, danakj wrote:
> can't see why the caller wouldn't just call container.remove_if() here

Done.

https://codereview.chromium.org/2723853002/diff/20001/base/stl_util.h#newcode200
base/stl_util.h:200: void STLErase(std::basic_string<CharT, Traits, Allocator>&
container,
On 2017/03/03 20:11:49, danakj wrote:
> I'm not sure where the whole STL prefix thing started, but I think these
should
> just be base::Erase and base::EraseIf (like base::MakeUnique, for example, or
> base::is_copy_assignable etc..)

Done.

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_set.h
File base/containers/flat_set.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:37: //    overload of base::EraseIf. Insertion is
harder - consider set operations
I'm not sure how to formulate this thought.

[Peter Kasting, 2017-03-03 23:27:31]

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_set.h
File base/containers/flat_set.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:37: //    overload of base::EraseIf. Insertion is
harder - consider set operations
On 2017/03/03 23:16:54, dyaroshev wrote:
> I'm not sure how to formulate this thought.

"Sets where mutating happens in bulk operations: to erase multiple elements, use
base::EraseIf() rather than repeated single-element removal."

[danakj, 2017-03-03 23:51:23]

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_set.h
File base/containers/flat_set.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:220: // Prefer the erase(std::remove(), end()) idiom
for deleting multiple
Like peter said above just say to use base::EraseIf() here

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:666: 
no whitespace here

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:670: std::remove_if(std::begin(container),
std::end(container), pred),
it's a bit less verbose to use container.begin() and container.end()

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
File base/containers/flat_set_unittest.cc (right):

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set_unittest.cc:1246: // STLEraseIf(FlatSet) is tested in
stl_util_unittests.cc
remove this, throw a test here if you like?

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode11
base/stl_util.h:11: #include <deque>
What is including all of these headers together so commonly going to do to
compile time, anything?

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode32
base/stl_util.h:32: for (auto it = std::begin(container); it !=
std::end(container);) {
you could use container.begin()/end() here too right, there's no arrays.

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode165
base/stl_util.h:165: std::remove(std::begin(container), std::end(container),
value),
same for these, you don't need std::begin/end right?

[dyaroshev, 2017-03-03 23:57:56]

Patchset #4 (id:100001) has been deleted

[dyaroshev, 2017-03-04 00:32:49]

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_set.h
File base/containers/flat_set.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:37: //    overload of base::EraseIf. Insertion is
harder - consider set operations
On 2017/03/03 23:27:30, Peter Kasting wrote:
> On 2017/03/03 23:16:54, dyaroshev wrote:
> > I'm not sure how to formulate this thought.
> 
> "Sets where mutating happens in bulk operations: to erase multiple elements,
use
> base::EraseIf() rather than repeated single-element removal."

Done.

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:220: // Prefer the erase(std::remove(), end()) idiom
for deleting multiple
On 2017/03/03 23:51:22, danakj wrote:
> Like peter said above just say to use base::EraseIf() here

Slightly extended it. Is it ok?

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:666: 
On 2017/03/03 23:51:22, danakj wrote:
> no whitespace here

Done.

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set.h:670: std::remove_if(std::begin(container),
std::end(container), pred),
On 2017/03/03 23:51:22, danakj wrote:
> it's a bit less verbose to use container.begin() and container.end()

Done.

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
File base/containers/flat_set_unittest.cc (right):

https://codereview.chromium.org/2723853002/diff/80001/base/containers/flat_se...
base/containers/flat_set_unittest.cc:1246: // STLEraseIf(FlatSet) is tested in
stl_util_unittests.cc
On 2017/03/03 23:51:22, danakj wrote:
> remove this, throw a test here if you like?

Done.

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode11
base/stl_util.h:11: #include <deque>
On 2017/03/03 23:51:22, danakj wrote:
> What is including all of these headers together so commonly going to do to
> compile time, anything?

Good point. I will do the measurements and come back to you.

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode32
base/stl_util.h:32: for (auto it = std::begin(container); it !=
std::end(container);) {
On 2017/03/03 23:51:22, danakj wrote:
> you could use container.begin()/end() here too right, there's no arrays.

Done.

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode165
base/stl_util.h:165: std::remove(std::begin(container), std::end(container),
value),
On 2017/03/03 23:51:22, danakj wrote:
> same for these, you don't need std::begin/end right?

Done.

[dyaroshev, 2017-03-04 00:33:22]

Description was changed from

==========
Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> STLErase
erase_if -> STLEraseIf

BUG=697235
==========

to

==========
Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> base::Erase
erase_if -> base::EraseIf

BUG=697235
==========

[dyaroshev, 2017-03-05 10:49:59]

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/2723853002/diff/80001/base/stl_util.h#newcode11
base/stl_util.h:11: #include <deque>
On 2017/03/04 00:32:49, dyaroshev wrote:
> On 2017/03/03 23:51:22, danakj wrote:
> > What is including all of these headers together so commonly going to do to
> > compile time, anything?
> 
> Good point. I will do the measurements and come back to you.

Doesn't seem to affect build times. Checked on mac. I've tried to provide as
much of the same environment as I could - nothing else was running, after recent
restart. Unfortunately I forgot -k parameter the second time but I don't think
it should significantly affect time.

// master -------------------------------------------------------------------

time ninja -k 1000000 -C out/gn/ chrome
...

real  172m47.963s
user  1292m6.310s
sys 61m58.296s

// with changes --------------------------------------------------------------

time ninja -C out/gn chrome
...

real  168m0.094s
user  1258m49.411s
sys 58m40.793s

[danakj, 2017-03-06 16:21:56]

LGTM

https://codereview.chromium.org/2723853002/diff/120001/base/containers/flat_s...
File base/containers/flat_set_unittest.cc (right):

https://codereview.chromium.org/2723853002/diff/120001/base/containers/flat_s...
base/containers/flat_set_unittest.cc:1246: // void EraseIf(flat_set)
This comment is exactly the test name, just remove it

[dyaroshev, 2017-03-06 21:13:52]

The CQ bit was checked by dyaroshev@yandex-team.ru

[dyaroshev, 2017-03-06 21:13:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2723853002/#ps130001
(title: "Review, round 4.")

[commit-bot: I haz the power, 2017-03-06 21:14:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 21:33:22]

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

[commit-bot: I haz the power, 2017-03-06 21:33:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dyaroshev, 2017-03-06 22:31:47]

The CQ bit was checked by dyaroshev@yandex-team.ru

[dyaroshev, 2017-03-06 22:31:47]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2723853002/#ps150001
(title: "Compilation on linux.")

[commit-bot: I haz the power, 2017-03-06 22:32:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 01:24:15]

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

[commit-bot: I haz the power, 2017-03-07 01:24:16]

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

[dyaroshev, 2017-03-07 06:28:54]

The CQ bit was checked by dyaroshev@yandex-team.ru

[commit-bot: I haz the power, 2017-03-07 06:29:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 09:27:21]

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

[commit-bot: I haz the power, 2017-03-07 09:27:22]

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

[dyaroshev, 2017-03-07 09:43:16]

On 2017/03/07 09:27:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

@reviewers: I'm failing with installer tests:

test_installer (retry summary) test_installer (retry summary)

failures:
__main__.InstallerTest.MigrateMultiStrandedBinariesOnInstall
__main__.InstallerTest.MigrateMultiStrandedBinariesOnUpdate
__main__.InstallerTest.ChromeSystemLevel
__main__.InstallerTest.MigrateMultiNoBinaries
__main__.InstallerTest.ChromeUserLevel
__main__.InstallerTest.ChromeSystemLevelUpdate
__main__.InstallerTest.MigrateMultiSimple
__main__.InstallerTest.ChromeUserLevelUpdate

Is everything alright with them? I don't see any obvious reason for my changes
to affect them.

[Peter Kasting, 2017-03-07 09:56:29]

On 2017/03/07 09:43:16, dyaroshev wrote:
> On 2017/03/07 09:27:22, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> 
> @reviewers: I'm failing with installer tests:
> 
> test_installer (retry summary) test_installer (retry summary)
> 
> failures:
> __main__.InstallerTest.MigrateMultiStrandedBinariesOnInstall
> __main__.InstallerTest.MigrateMultiStrandedBinariesOnUpdate
> __main__.InstallerTest.ChromeSystemLevel
> __main__.InstallerTest.MigrateMultiNoBinaries
> __main__.InstallerTest.ChromeUserLevel
> __main__.InstallerTest.ChromeSystemLevelUpdate
> __main__.InstallerTest.MigrateMultiSimple
> __main__.InstallerTest.ChromeUserLevelUpdate
> 
> Is everything alright with them? I don't see any obvious reason for my changes
> to affect them.

I saw something like these earlier today in my tryruns.  I think something is
busted.  Didn't find an obvious candidate in crbug.com with five seconds of
searching, but I didn't try very hard.

Going to bed for the moment, but feel free to file bugs on this?  In any case, I
suspect it's not your patch's fault.

[dyaroshev, 2017-03-07 10:03:00]

On 2017/03/07 09:56:29, Peter Kasting wrote:
> On 2017/03/07 09:43:16, dyaroshev wrote:
> > On 2017/03/07 09:27:22, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> > 
> > @reviewers: I'm failing with installer tests:
> > 
> > test_installer (retry summary) test_installer (retry summary)
> > 
> > failures:
> > __main__.InstallerTest.MigrateMultiStrandedBinariesOnInstall
> > __main__.InstallerTest.MigrateMultiStrandedBinariesOnUpdate
> > __main__.InstallerTest.ChromeSystemLevel
> > __main__.InstallerTest.MigrateMultiNoBinaries
> > __main__.InstallerTest.ChromeUserLevel
> > __main__.InstallerTest.ChromeSystemLevelUpdate
> > __main__.InstallerTest.MigrateMultiSimple
> > __main__.InstallerTest.ChromeUserLevelUpdate
> > 
> > Is everything alright with them? I don't see any obvious reason for my
changes
> > to affect them.
> 
> I saw something like these earlier today in my tryruns.  I think something is
> busted.  Didn't find an obvious candidate in http://crbug.com with five
seconds of
> searching, but I didn't try very hard.
> 
> Going to bed for the moment, but feel free to file bugs on this?  In any case,
I
> suspect it's not your patch's fault.

Oh, yeah, I could search for a bug) Here it is: crbug.com/698869

[dyaroshev, 2017-03-07 22:24:41]

The CQ bit was checked by dyaroshev@yandex-team.ru

[commit-bot: I haz the power, 2017-03-07 22:26:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 01:24:54]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1488925481630570,
"parent_rev": "6b0bfdf0e66bfeb8cf5240ef8aea607c1828e8e6", "commit_rev":
"5f0ff79896d3a6aa7a93de31cfa192c2312e5d7f"}

[commit-bot: I haz the power, 2017-03-08 01:26:06]

Description was changed from

==========
Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> base::Erase
erase_if -> base::EraseIf

BUG=697235
==========

to

==========
Implementing erase/erase_if functions from library fundamentals ts:
http://en.cppreference.com/w/cpp/experimental/lib_extensions_2

erase -> base::Erase
erase_if -> base::EraseIf

BUG=697235

Review-Url: https://codereview.chromium.org/2723853002
Cr-Commit-Position: refs/heads/master@{#455322}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f0ff79896d3a6aa7a93de31cfa1...
==========

[commit-bot: I haz the power, 2017-03-08 01:26:13]

Committed patchset #6 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/5f0ff79896d3a6aa7a93de31cfa1...