scoped_refptr: support opaque pointed-to classes via extern template

scoped_refptr: support opaque pointed-to classes via extern template

The C++ spec says the compiler may instantiate inline template
function definitions (e.g., member functions defined inline within a
templated class, even without an explicit "inline" keyword) even if
there's an "extern template class Foo<Bar>;" declaration.

In the case of scoped_refptr, if we merely have a forward declaration
for a class like "class Opaque;", then "extern template class
scoped_refptr<Opaque>;" will still cause compiler errors because of
scoped_refptr's inline member functions that try to call AddRef() and
Release().

To solve this, this CL introduces static helper methods that are
defined out-of-line to provide access to the AddRef() and Release()
methods.

BUG=414363
Committed: https://crrev.com/3d16d9f6566c5fc82eb23ab7b09217a59fb0957b
Cr-Commit-Position: refs/heads/master@{#298991}

[mdempsky, 2014-10-02 02:53:21]

mdempsky@chromium.org changed reviewers:
+ jyasskin@chromium.org, willchan@chromium.org

[willchan no longer on Chromium, 2014-10-03 00:19:24]

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

[willchan no longer on Chromium, 2014-10-03 00:19:24]

302=>Dana

[mdempsky, 2014-10-03 18:19:51]

On 2014/10/03 00:19:24, willchan OOO until 03-22-15 wrote:
> 302=>Dana

danakj: PTAL. This is a tiny refactoring that will allow me to move
implementation details out of some Linux sandbox header files.

[Jeffrey Yasskin, 2014-10-03 20:29:25]

Ick, but LGTM.

[danakj, 2014-10-09 00:06:58]

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h#ne...
base/memory/ref_counted.h:370: // Otherwise the compiler will complain that
Opaque is an incomplete type.
Can you add a test that demonstrates this working?

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h#ne...
base/memory/ref_counted.h:377: ptr->AddRef();
What does the code look like with/out this change in an optimized build? Is
there any difference? (I expect not but please check.)

[mdempsky, 2014-10-09 00:21:33]

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h#ne...
base/memory/ref_counted.h:370: // Otherwise the compiler will complain that
Opaque is an incomplete type.
On 2014/10/09 00:06:57, danakj wrote:
> Can you add a test that demonstrates this working?

Sure.  Do you know if there are any existing idioms for testing
cross-compilation-unit behavior I can follow?

My inclination is to write something like:

  ref_counted_unittest_helper.h:
    class OpaqueHelper;
    scoped_refptr<OpaqueHelper> MakeOpaque();
    void UseOpaque(scoped_refptr<OpaqueHelper>);
    extern template class scoped_refptr<OpaqueHelper>;

  ref_counted_unittest_helper.cc:
    class OpaqueHelper : public base::RefCounted<OpaqueHelper> {
      ...
    };
    scoped_refptr<OpaqueHelper> MakeOpaque() {
      return new OpaqueHelper();
    }
    void UseOpaque(scoped_refptr<OpaqueHelper> p) {
      ...
    }
    template class scoped_refptr<Opaque>;

  ref_counted_unittest.cc:
    TEST(RefCountedUnitTest, ScopedRefPtrToOpaque) {
      scoped_refptr<OpaqueHelper> p = MakeOpaque();
      UseOpaque(p);

      scoped_refptr<OpaqueHelper> q;
      q = p;
      UseOpaque(p);
      UseOpaque(q);
    }

and add the new _helper files to only the test target.  Does that sound
reasonable?

[danakj, 2014-10-09 00:31:48]

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h#ne...
base/memory/ref_counted.h:370: // Otherwise the compiler will complain that
Opaque is an incomplete type.
On 2014/10/09 00:21:33, mdempsky wrote:
> On 2014/10/09 00:06:57, danakj wrote:
> > Can you add a test that demonstrates this working?
> 
> Sure.  Do you know if there are any existing idioms for testing
> cross-compilation-unit behavior I can follow?
> 
> My inclination is to write something like:
> 
>   ref_counted_unittest_helper.h:
>     class OpaqueHelper;
>     scoped_refptr<OpaqueHelper> MakeOpaque();
>     void UseOpaque(scoped_refptr<OpaqueHelper>);
>     extern template class scoped_refptr<OpaqueHelper>;
> 
>   ref_counted_unittest_helper.cc:
>     class OpaqueHelper : public base::RefCounted<OpaqueHelper> {
>       ...
>     };
>     scoped_refptr<OpaqueHelper> MakeOpaque() {
>       return new OpaqueHelper();
>     }
>     void UseOpaque(scoped_refptr<OpaqueHelper> p) {
>       ...
>     }
>     template class scoped_refptr<Opaque>;
> 
>   ref_counted_unittest.cc:
>     TEST(RefCountedUnitTest, ScopedRefPtrToOpaque) {
>       scoped_refptr<OpaqueHelper> p = MakeOpaque();
>       UseOpaque(p);
> 
>       scoped_refptr<OpaqueHelper> q;
>       q = p;
>       UseOpaque(p);
>       UseOpaque(q);
>     }
> 
> and add the new _helper files to only the test target.  Does that sound
> reasonable?

That sounds like how I'd do it, putting the helper files in base/test/. But is
extern template class still needed to make this compile after this change?

[mdempsky, 2014-10-09 00:52:48]

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h#ne...
base/memory/ref_counted.h:370: // Otherwise the compiler will complain that
Opaque is an incomplete type.
On 2014/10/09 00:31:47, danakj wrote:
> That sounds like how I'd do it, putting the helper files in base/test/.

SGTM

> But is extern template class still needed to make this compile after this
change?

Yes. The "extern template class" declaration informs the compiler it can assume
another compilation unit will instantiate that template class; otherwise, it has
to conservatively instantiate all needed templates itself, and would complain
that ptr->AddRef() is dereferencing a pointer to an incomplete type.

[danakj, 2014-10-09 01:03:20]

OK. It'd be extern template class Opaque; though, not extern template class
scoped_refptr<Opaque>;?
On Oct 8, 2014 8:52 PM, <mdempsky@chromium.org> wrote:

>
> https://codereview.chromium.org/623633002/diff/1/base/memory/ref_counted.h
> File base/memory/ref_counted.h (right):
>
> https://codereview.chromium.org/623633002/diff/1/base/
> memory/ref_counted.h#newcode370
> base/memory/ref_counted.h:370: // Otherwise the compiler will complain
> that Opaque is an incomplete type.
> On 2014/10/09 00:31:47, danakj wrote:
>
>> That sounds like how I'd do it, putting the helper files in
>>
> base/test/.
>
> SGTM
>
>  But is extern template class still needed to make this compile after
>>
> this change?
>
> Yes. The "extern template class" declaration informs the compiler it can
> assume another compilation unit will instantiate that template class;
> otherwise, it has to conservatively instantiate all needed templates
> itself, and would complain that ptr->AddRef() is dereferencing a pointer
> to an incomplete type.
>
> https://codereview.chromium.org/623633002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2014-10-09 03:55:57]

On 2014/10/09 01:03:20, danakj wrote:
> OK. It'd be extern template class Opaque; though, not extern template class
> scoped_refptr<Opaque>;?

It's "extern template class scoped_refptr<Opaque>;" to match the explicit
instantiation from "template class scoped_refptr<Opaque>;".  ("extern template
class Opaque;" would be ill-formed since Opaque isn't a templated class.)

It might be easier to discuss with something concrete, so I've uploaded Patch
Set 2 to implement the discussed test case.  I've at least confirmed that it
works locally on Ubuntu with a component=shared_library build; hopefully the
trybots (particularly Windows :-/) are happy with it.

PTAL

[danakj, 2014-10-09 16:56:21]

Ah, it's the scoped_refptr<T>::AddRef that becomes non-inline, right. I was
thinking it was the T::AddRef that was not inline, but it's the call to it.

This LG if it doesn't change the code we generate in the non-extern template
case.

[mdempsky, 2014-10-09 19:46:02]

On 2014/10/09 16:56:21, danakj wrote:
> This LG if it doesn't change the code we generate in the non-extern template
> case.

I picked
out/Release/obj/chrome/browser/extensions/api/debugger/browser_extensions.debugger_api.o
as a random test file because
chrome/browser/extensions/api/debugger/debugger_api.cc includes calls to the
content public API  content::DevToolsAgentHost::GetForId() which returns a
scoped_refptr<DevToolsAgentHost> where DevToolsAgentHost is a CONTEXT_EXPORT
class.  I also confirmed from the .o file that it includes calls to AddRef and
Release.

Building that file with or without this diff generates identical object files.

I also built the full out/Release/chrome target with and without.  With this
change, chrome's text segment (as reported by the "size" utility) grew ~40
bytes, whereas the overall executable size (as reported by "ls") shrunk by ~400
bytes.  I haven't investigated why; but out of a 50MB executable, I think those
deltas are pretty negligible.

[danakj, 2014-10-09 20:10:18]

LGTM thanks!

[mdempsky, 2014-10-09 20:14:47]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-10-09 20:16:28]

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

[commit-bot: I haz the power, 2014-10-09 22:17:16]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-10-09 22:17:55]

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