Remove the free_ member of scoped_ptr_malloc.

Remove the free_ member of scoped_ptr_malloc.

Conceptually, the function to call is known at compile time.
There is no reason to save it into a static variable.  It turns out
doing causes the compiler to emit a static initializer to track whether
free_ has been initialized.

This change removes static initializers from eight .o files on my local
Release build.

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

[Evan Martin, 2011-08-29 21:34:04]

I'm a little worried that some caller is relying on us initializing the FreeProc
at startup.  But I also think such a caller is crazy.

I believe this search finds the users who pass in a FreeProc:
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=scop...

You'll note some of them are too complicated to allow us to just use a plain
function pointer as the second template argument.

$ wc -l release-{before,after}.ctors
270 release-before.ctors
262 release-after.ctors


$ diff -u <(cut -d' ' -f3 release-before.ctors) <(cut -d' ' -f3
release-after.ctors) | grep ^- --- /dev/fd/63	2011-08-29 14:28:07.926603582
-0700
-_GLOBAL__I_browser_render_process_host.cc
-_GLOBAL__I_nsNSSCertHelper.cpp
-_GLOBAL__I_prefix_set.cc
-_GLOBAL__I_safe_browsing_store_file.cc
-_GLOBAL__I_socket_stream_host.cc
-_GLOBAL__I_visitedlink_master.cc
-_GLOBAL__I_worker_process_host.cc
-_GLOBAL__I_x509_certificate_model_nss.cc

[Mark Mentovai, 2011-08-29 21:39:43]

This change needs a BUG=.

I may have filed a bug about this once.

[Mark Mentovai, 2011-08-29 21:40:11]

LGTM.

The history of this (upstream) is ugly.

[Evan Martin, 2011-08-29 21:40:57]

On 2011/08/29 21:39:43, Mark Mentovai wrote:
> This change needs a BUG=.

What for?

> The history of this (upstream) is ugly.

Is it important for this change?  I won't do it if there's a good reason not to.

[Mark Mentovai, 2011-08-29 21:42:07]

evan@chromium.org wrote:
> On 2011/08/29 21:39:43, Mark Mentovai wrote:
>>
>> This change needs a BUG=.
>
> What for?

To have something interesting to point to in the future.

>> The history of this (upstream) is ugly.
>
> Is it important for this change?  I won't do it if there's a good reason not
> to.

No, I’m just pointing out that I investigated why we had this at one
point and the upstream history was gross.

[awong, 2011-08-29 21:45:11]

LGTM

with one mild suggestion.

http://codereview.chromium.org/7795008/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/7795008/diff/1/base/memory/scoped_ptr.h#newcod...
base/memory/scoped_ptr.h:288: FreeProc()(ptr_);
nit: What about saving it into a temporary before executing.

FreeProc free_proc;
free_proc(ptr_);

Will confusing step-into less in a debugger...

[Mark Mentovai, 2011-08-29 21:48:16]

Yes, I did file a bug about this.

BUG=87171

[Evan Martin, 2011-08-29 21:55:39]

bug updated, code updated to ajwong suggestion

[Mark Mentovai, 2011-08-29 22:05:04]

LGTM

[darin (slow to review), 2011-08-29 23:32:10]

Feels like deja-vu.  I remember wondering about why this was a static
instance...  what kept me from making this very same change?...

http://codereview.chromium.org/7795008/diff/6001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/7795008/diff/6001/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:296: FreeProc free_proc;
unrelated to this change, but since free_proc(ptr_) could have side-effects,
should we consider updating ptr_ before calling free_proc?

  C* doomed = ptr_;
  ptr_ = p;
  FreeProc free_proc;
  free_proc(doomed);

[joth, 2011-10-17 15:30:52]

Glad this made it in eventually!

On 2011/08/29 23:32:10, darin wrote:
> Feels like deja-vu.  I remember wondering about why this was a static
> instance...  what kept me from making this very same change?...
> 
> 
Sorry about the deja-vu -- that was my bad. We had an email thread about it last
year (I drafted http://codereview.chromium.org/3031001/ as an illustration for
that thread) but we got bogged down in discussion whether the divergence from
other (i.e. google internal) versions was bad. I took up the conversation over
there (see http://cl/16515990) but it died a death.


Anyway, should we merge this change into the other chrome copies of
scoped_ptr.h? That way at least we'll be self-consistent.

gpu/command_buffer/common/scoped_ptr.h
third_party/cld/base/scoped_ptr.h 
third_party/cld/base/scoped_ptr.h 
(and third_party/libjingle/mods-since-v0_4_0.diff)

[darin (slow to review), 2011-10-17 15:39:23]

On Mon, Oct 17, 2011 at 8:30 AM, <joth@chromium.org> wrote:

> Glad this made it in eventually!
>
> On 2011/08/29 23:32:10, darin wrote:
>
>> Feels like deja-vu.  I remember wondering about why this was a static
>> instance...  what kept me from making this very same change?...
>>
>
>
> Sorry about the deja-vu -- that was my bad. We had an email thread about it
> last
> year (I drafted
http://codereview.chromium.**org/3031001/<http://codereview.chromium.org/3031...
an illustration for
> that thread) but we got bogged down in discussion whether the divergence
> from
> other (i.e. google internal) versions was bad. I took up the conversation
> over
> there (see http://cl/16515990) but it died a death.
>
>
Ah!


>
> Anyway, should we merge this change into the other chrome copies of
> scoped_ptr.h? That way at least we'll be self-consistent.
>
> gpu/command_buffer/common/**scoped_ptr.h
> third_party/cld/base/scoped_**ptr.h
> third_party/cld/base/scoped_**ptr.h
> (and third_party/libjingle/mods-**since-v0_4_0.diff)
>
>
I think native client may have a copy too!

Merging it into the other copies seems like a nice thing to do.

-Darin



>
>
>
http://codereview.chromium.**org/7795008/<http://codereview.chromium.org/7795...
>