Cleanup base/stl_util

some cleanup for base/stl_util

removed unused/irrelevant functions
removed irrelevant comments
Moved stl_util-inl.h => stl_util.h

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93110

[Denis Lagno, 2011-07-14 07:49:24]

please take a look

[brettw, 2011-07-14 15:10:42]

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h
File base/stl_util-inl.h (right):

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h#newcode12
base/stl_util-inl.h:12: #include <assert.h>
What was the reason for this assert change? Wouldn't cassert be preferred for
C++ code?

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h#newcode23
base/stl_util-inl.h:23: template<class T> void STLClearObject(T* obj, size_t
limit = 1 << 6) {
I don't understand what your goal with this change was, and I question why our
implementation should be different than google3/util/gtl/stl_util.h

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h#newcode106
base/stl_util-inl.h:106: return &v->front();
I wouldn't change these, I think the generated code should be the same, and it
makes us diverge unnecessarily from the google3 version.

[Denis Lagno, 2011-07-14 16:45:26]

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h
File base/stl_util-inl.h (right):

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h#newcode12
base/stl_util-inl.h:12: #include <assert.h>
On 2011/07/14 15:10:42, brettw wrote:
> What was the reason for this assert change? Wouldn't cassert be preferred for
> C++ code?

1st reason is that it is inconsistent: using <cassert> and <string.h>
simultaneously.  So one of them should be changed .

2nd reason is that in this case it is just confusing: generally <cname> headers
export their stuff in namespace std, however code accesses assert not
std::assert -- that is because assert is actually preprocessor macro not a
function.

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h#newcode23
base/stl_util-inl.h:23: template<class T> void STLClearObject(T* obj, size_t
limit = 1 << 6) {
On 2011/07/14 15:10:42, brettw wrote:
> I don't understand what your goal with this change was, and I question why our
> implementation should be different than google3/util/gtl/stl_util.h

My main goal was to get rid of STLClearIfBig function -- its name is just
misleading: in fact it clears unconditionally (not only big objects), just for
big objects it tries harder to reclaim occupied memory.

On second thought I decided just to move previous functionality of STLClearIfBig
into STLClearObject -- because they actually have the same semantics (clear
object) with different memory reclaiming behaviour.

But ok, I've just reverted to previous behaviour of STLClearObject.

http://codereview.chromium.org/7342047/diff/2001/base/stl_util-inl.h#newcode106
base/stl_util-inl.h:106: return &v->front();
On 2011/07/14 15:10:42, brettw wrote:
> I wouldn't change these, I think the generated code should be the same, and it
> makes us diverge unnecessarily from the google3 version.

Done.

[brettw, 2011-07-15 05:32:37]

The background is this file was copied from the server side long ago. I kind of
hate this file, many of the functions are weird or difficult to understand, and
there's only a couple I'd keep I was doing it over. Many uses would be more
clear if the calling code just wrote the logic they needed. Really the only
redeeming thing about it file is that it's the same as the server one is (well,
as it used to be).

It would be OK with me if we changed this to sync with the latest google3 copy.
I'd also be OK if we deleted the stupid thing it and moved the ~4 useful
functions into a more appropriately named file. But absent bugs, my inclination
would always be to avoid messing with the file which would generate a further
diff from the rest of Google.

Given that, do you have thoughts about what we should do?

http://codereview.chromium.org/7342047/diff/6001/base/stl_util-inl.h
File base/stl_util-inl.h (right):

http://codereview.chromium.org/7342047/diff/6001/base/stl_util-inl.h#newcode118
base/stl_util-inl.h:118: // which may be not null-terminated. Writing through
this pointer will
Seems like the old comment was fine (and grammatical).

[Denis Lagno, 2011-07-18 16:12:38]

I see.  Initially I was not aware that it is from google3.

Looking at the code: current util/gtl/stl_util.h quite diverged from current
chromium base/stl_util-inl.h

I propose following: do not look at this code as on something sacred: just do
not modify code without need but do remove freely.

I've moved it into stl_util.h because in my understanding current usage of
stl_util-inl.h is incorrect: file-inl.h were intended as just a complement to
plain file.h: so file.h should contain interface (declaration+comments) and
file-inl.h contains inline definitions; file.h includes file-inl.h but all
source files are expected to include file.h not file-inl.h

In my understanding file-inl.h without corresponding file.h is not an expected
usage.

I kept names of functions and their code intact, however I removed unused
functions and I removed some irrelevant comments.
I have not added any new functions from google3 since I do not see a need to do
it ATM (and AFAIU it will require some discussion/permission about opening this
code).

http://codereview.chromium.org/7342047/diff/6001/base/stl_util-inl.h
File base/stl_util-inl.h (right):

http://codereview.chromium.org/7342047/diff/6001/base/stl_util-inl.h#newcode118
base/stl_util-inl.h:118: // which may be not null-terminated. Writing through
this pointer will
On 2011/07/15 05:32:38, brettw wrote:
> Seems like the old comment was fine (and grammatical).

Reverted.

[brettw, 2011-07-18 18:28:54]

I'm happy treating it as non-sacred, but then I question the usefulness of
things like STLAssignToVector which looks like it's only used once, and could be
implemented as a std::copy call.

Don't bother at all with the -inl file in your patch, see
https://wiki/twiki/bin/view/Nonconf/InlFiles
for how we're now just supposed to be moving away from these

[Denis Lagno, 2011-07-19 10:52:18]

On 2011/07/18 18:28:54, brettw wrote:
> I'm happy treating it as non-sacred, but then I question the usefulness of
> things like STLAssignToVector which looks like it's only used once, and could
be
> implemented as a std::copy call.

Removed STLAssign* functions

> Don't bother at all with the -inl file in your patch, see
> https://wiki/twiki/bin/view/Nonconf/InlFiles
> for how we're now just supposed to be moving away from these

Problem is with third_party/hunspell
third_party/hunspell/google.patch adds #include "base/stl_util-inl.h"
And unfortunately this is outside of chromium repository.
So I need to either fix it outside of chromium repository or add workaround (as
I did).

[Denis Lagno, 2011-07-19 12:46:38]

On 2011/07/19 10:52:18, Denis Lagno wrote:
> Problem is with third_party/hunspell
> third_party/hunspell/google.patch adds #include "base/stl_util-inl.h"
> And unfortunately this is outside of chromium repository.
> So I need to either fix it outside of chromium repository or add workaround
(as
> I did).

oh, I see.  deps/third_party/hunspell is in chromium repository, however it is
outside my git checkout and I do not quite understand how to make changes to it.

[brettw, 2011-07-19 15:55:15]

On Tue, Jul 19, 2011 at 5:46 AM,  <dilmah@chromium.org> wrote:
> On 2011/07/19 10:52:18, Denis Lagno wrote:
>>
>> Problem is with third_party/hunspell
>> third_party/hunspell/google.patch adds #include "base/stl_util-inl.h"
>> And unfortunately this is outside of chromium repository.
>> So I need to either fix it outside of chromium repository or add
>> workaround
>
> (as
>>
>> I did).
>
> oh, I see.  deps/third_party/hunspell is in chromium repository, however it
> is
> outside my git checkout and I do not quite understand how to make changes to
> it.

This kind of sucks, but it's pretty easy as far as these things go:
check out that path with svn, do the change, check it in. Then set the
DEPS file in Chrome to refer to your new revision.

Brett

[brettw, 2011-07-19 16:01:04]

It would be nice to preserve the change history. Can you save off your
stl_util.h, do an svn cp stl_util-inl.h stl_util.h, and then put your modified
one back over the top?

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h
File base/stl_util.h (right):

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h#newcode139
base/stl_util.h:139: inline bool HashSetEquality(const HashSet& set_a, const
HashSet& set_b) {
It looks like nobody uses this and it can be deleted.

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h#newcode150
base/stl_util.h:150: inline bool HashMapEquality(const HashMap& map_a, const
HashMap& map_b) {
It looks like nobody uses this and it can be deleted.

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h#newcode231
base/stl_util.h:231: std::vector<T> SetToVector(const std::set<T>& values) {
Can you remove this? It looks like nobody calls it.

[Denis Lagno, 2011-07-19 18:31:00]

On 2011/07/19 16:01:04, brettw wrote:
> It would be nice to preserve the change history. Can you save off your
> stl_util.h, do an svn cp stl_util-inl.h stl_util.h, and then put your modified
> one back over the top?

I have git-svn checkout.  svn does not work on it.
I hope git cl dcommit should do the best to preserve svn history.

In any case git log --follow and git blame are tracking old history of
stl_util.h correctly.



On 2011/07/19 15:55:15, brettw wrote:
> check out that path with svn, do the change, check it in. Then set the
> DEPS file in Chrome to refer to your new revision.

ok, I've committed changes to deps/third_party/hunspell/src/hunspell/hashmgr.hxx
and deps/third_party/hunspell/google.deps
and changed DEPS in this CL.

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h
File base/stl_util.h (right):

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h#newcode139
base/stl_util.h:139: inline bool HashSetEquality(const HashSet& set_a, const
HashSet& set_b) {
On 2011/07/19 16:01:04, brettw wrote:
> It looks like nobody uses this and it can be deleted.

Done.

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h#newcode150
base/stl_util.h:150: inline bool HashMapEquality(const HashMap& map_a, const
HashMap& map_b) {
On 2011/07/19 16:01:04, brettw wrote:
> It looks like nobody uses this and it can be deleted.

Done.

http://codereview.chromium.org/7342047/diff/16001/base/stl_util.h#newcode231
base/stl_util.h:231: std::vector<T> SetToVector(const std::set<T>& values) {
On 2011/07/19 16:01:04, brettw wrote:
> Can you remove this? It looks like nobody calls it.

it was used once in printing/page_range.cc

Removed.

[brettw, 2011-07-19 20:05:00]

LGTM, but please update the base.gypi to remove the old file and add the new
one.

[joth, 2011-07-20 11:38:47]

Couple drive-by comments - if you agree with any of these change suggestions I'm
happy to pull together a CL for them.

Cheers!

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h
File base/stl_util.h (right):

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode12
base/stl_util.h:12: #include <string.h>  // for memcpy
we don't actually call memcpy anymore.

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode90
base/stl_util.h:90: }
fwiw server now has this as
  while (begin != end) {
    ForwardIterator temp = begin;
    ++begin;
    delete temp->second;
  }
}
(with comment to explain why - 'first' maybe a pointer into 'second' which could
invalidate the iterator)

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode99
base/stl_util.h:99: return &*v->begin();
this always seems to me to be a quite obscure non-portable optimization of
minimal benefit: only having the #else version would make this method a whole
load easier to understand.

Thoughts on removing this, or at least adding a test case for this (passing in
empty vector) to ensure all versions of STL we use allow this optimization?

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode125
base/stl_util.h:125: // already work on all current implementations.
FWIW, C++ 2008 does now clarify this:

21.3.1 basic_string general requirements
5 The char-like objects in a basic_string object shall be stored contiguously.
That is, for any basic_string
object s, the identity &*(s.begin() + n) == &*s.begin() + n shall hold for all
values of n such that 0 <= n < s.size()

[Denis Lagno, 2011-07-20 12:01:13]

On 2011/07/20 11:38:47, joth wrote:
> Couple drive-by comments - if you agree with any of these change suggestions
I'm
> happy to pull together a CL for them.

sounds reasonable.

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h
File base/stl_util.h (right):

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode12
base/stl_util.h:12: #include <string.h>  // for memcpy
On 2011/07/20 11:38:48, joth wrote:
> we don't actually call memcpy anymore.

thanks for noticing.

And BTW we do not call assert anymore as well.

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode90
base/stl_util.h:90: }
On 2011/07/20 11:38:48, joth wrote:
> fwiw server now has this as
>   while (begin != end) {
>     ForwardIterator temp = begin;
>     ++begin;
>     delete temp->second;
>   }
> }
> (with comment to explain why - 'first' maybe a pointer into 'second' which
could
> invalidate the iterator)

yes, it looks better to sync with server.

[Denis Lagno, 2011-07-20 12:54:35]

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h
File base/stl_util.h (right):

http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode99
base/stl_util.h:99: return &*v->begin();
On 2011/07/20 11:38:48, joth wrote:
> this always seems to me to be a quite obscure non-portable optimization of
> minimal benefit: only having the #else version would make this method a whole
> load easier to understand.
> 
> Thoughts on removing this, or at least adding a test case for this (passing in
> empty vector) to ensure all versions of STL we use allow this optimization?

agree, chromium (client) and server differs in that we target much larger number
of compilers/environments so it's much more important for us to be standards
compliant.

[joth, 2011-07-20 15:43:08]

On 20 July 2011 13:54, <dilmah@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7342047/diff/31001/base/**stl_util.h<http://...
> File base/stl_util.h (right):
>
> http://codereview.chromium.**org/7342047/diff/31001/base/**
>
stl_util.h#newcode99<http://codereview.chromium.org/7342047/diff/31001/base/stl_util.h#newcode99>
> base/stl_util.h:99: return &*v->begin();
> On 2011/07/20 11:38:48, joth wrote:
>
>> this always seems to me to be a quite obscure non-portable
>>
> optimization of
>
>> minimal benefit: only having the #else version would make this method
>>
> a whole
>
>> load easier to understand.
>>
>
>  Thoughts on removing this, or at least adding a test case for this
>>
> (passing in
>
>> empty vector) to ensure all versions of STL we use allow this
>>
> optimization?
>
> agree, chromium (client) and server differs in that we target much
> larger number of compilers/environments so it's much more important for
> us to be standards compliant.
>
>
OK, patch upload -> http://codereview.chromium.org/7465001
Thanks!