Remove implicit conversions from scoped_refptr to T* in gpu/

Committed: https://crrev.com/3d7ce9f4a3fd04b148bd9afc128965b020525410
Cr-Commit-Position: refs/heads/master@{#291805}

[dcheng, 2014-08-25 21:37:03]

dcheng@chromium.org changed reviewers:
+ sievers@chromium.org

[dcheng, 2014-08-25 21:37:03]

Unfortunately, we can't make scoped_refptr testable until the conversion
operator has been removed. While it would be possible to disable the conversion
operator and make it testable in the same patch, the automated tool tries to
skip rewriting code where manual cleanups would be useful. This includes things
like:

// What if ReturnsScopedRefPtr() creates a new object internally?
if (ReturnsScopedRefPtr())

Foo* Bar() {
  const scoped_refptr<Foo>& f = ReturnsScopedRefPtr();
  return f;  // Does the return type need to be rewritten? Or is .get() safe?
}

Doing this in one patch would make catching some of these instances (in
particular, a boolean test on a temporary scoped_refptr) more difficult. As a
result, the work is being split into several steps:
Step 1: Rewrite all implicit conversions to be explicit (or rewrite the receiver
type to scoped_refptr<T>, if the tool believes the conversion to be unsafe).
Step 2: Once a platform compiles cleanly without the conversion operator, #ifdef
the conversion operator out.
Step 3: Make scoped_refptr testable on that platform.
Step 4: Remove unnecessary calls to .get().

[no sievers, 2014-08-25 21:57:16]

lgtm

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2320:
EXPECT_CALL(*image.get(), BindTexImage(GL_TEXTURE_2D))
hmm scoped_ptr<> has an operator*. Should scoped_refptr<> have that too?

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:270: DCHECK(!share_group
|| service_.get() == share_group->service_.get());
same here, what if only one was a raw pointer (other scoped_refptr)? should that
operator be supported?

[dcheng, 2014-08-25 22:02:55]

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2320:
EXPECT_CALL(*image.get(), BindTexImage(GL_TEXTURE_2D))
On 2014/08/25 21:57:15, sievers wrote:
> hmm scoped_ptr<> has an operator*. Should scoped_refptr<> have that too?

That's a good question. Conceptually, the closest thing to scoped_refptr<T> is
shared_ptr<T>, which also has an operator*. It might make sense to add, but it
probably won't be considered as part of this work, which is already painful
enough as it is =(

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:270: DCHECK(!share_group
|| service_.get() == share_group->service_.get());
On 2014/08/25 21:57:15, sievers wrote:
> same here, what if only one was a raw pointer (other scoped_refptr)? should
that
> operator be supported?

This is kind of silly. We should support comparisons between two scoped_refptrs.
I've removed this.

[no sievers, 2014-08-25 22:10:58]

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:270: DCHECK(!share_group
|| service_.get() == share_group->service_.get());
On 2014/08/25 22:02:54, dcheng (OOO) wrote:
> On 2014/08/25 21:57:15, sievers wrote:
> > same here, what if only one was a raw pointer (other scoped_refptr)? should
> that
> > operator be supported?
> 
> This is kind of silly. We should support comparisons between two
scoped_refptrs.
> I've removed this.

scoped_ptr allows comparison between scoped_ptr<T> and T*, but not between
scoped_ptr<U> and scoped_ptr<T>. It says because
a) it doesn't make sense if U and T are different types
b) there should never be two scoped_ptrs pointing to the same object

b) obviously doesn't apply to scoped_refptr, but not sure about a)

[dcheng, 2014-08-25 22:19:55]

On 2014/08/25 at 22:10:58, sievers wrote:
>
https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
> File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
> gpu/command_buffer/service/in_process_command_buffer.cc:270:
DCHECK(!share_group || service_.get() == share_group->service_.get());
> On 2014/08/25 22:02:54, dcheng (OOO) wrote:
> > On 2014/08/25 21:57:15, sievers wrote:
> > > same here, what if only one was a raw pointer (other scoped_refptr)?
should
> > that
> > > operator be supported?
> > 
> > This is kind of silly. We should support comparisons between two
scoped_refptrs.
> > I've removed this.
> 
> scoped_ptr allows comparison between scoped_ptr<T> and T*, but not between
scoped_ptr<U> and scoped_ptr<T>. It says because
> a) it doesn't make sense if U and T are different types
> b) there should never be two scoped_ptrs pointing to the same object
> 
> b) obviously doesn't apply to scoped_refptr, but not sure about a)

It's unclear. The standard library supports comparisons for unique_ptr. Let me
ask what people think, and get back to you on this.

In the meanwhile, would you be OK with submitting the CL as is?

[no sievers, 2014-08-25 22:24:26]

On 2014/08/25 22:19:55, dcheng (OOO) wrote:
> On 2014/08/25 at 22:10:58, sievers wrote:
> >
>
https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
> > File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> > 
> >
>
https://codereview.chromium.org/500243002/diff/1/gpu/command_buffer/service/i...
> > gpu/command_buffer/service/in_process_command_buffer.cc:270:
> DCHECK(!share_group || service_.get() == share_group->service_.get());
> > On 2014/08/25 22:02:54, dcheng (OOO) wrote:
> > > On 2014/08/25 21:57:15, sievers wrote:
> > > > same here, what if only one was a raw pointer (other scoped_refptr)?
> should
> > > that
> > > > operator be supported?
> > > 
> > > This is kind of silly. We should support comparisons between two
> scoped_refptrs.
> > > I've removed this.
> > 
> > scoped_ptr allows comparison between scoped_ptr<T> and T*, but not between
> scoped_ptr<U> and scoped_ptr<T>. It says because
> > a) it doesn't make sense if U and T are different types
> > b) there should never be two scoped_ptrs pointing to the same object
> > 
> > b) obviously doesn't apply to scoped_refptr, but not sure about a)
> 
> It's unclear. The standard library supports comparisons for unique_ptr. Let me
> ask what people think, and get back to you on this.
> 
> In the meanwhile, would you be OK with submitting the CL as is?

sure, sounds good.

[dcheng, 2014-08-25 22:25:20]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:27:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/500243002/20001

[commit-bot: I haz the power, 2014-08-26 00:16:35]

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

[commit-bot: I haz the power, 2014-08-26 00:16:36]

Failed to commit the patch.

[dcheng, 2014-08-26 00:26:59]

Committed patchset #2 to pending queue manually as 1abcbd9 (presubmit
successful).

[commit-bot: I haz the power, 2014-09-10 02:39:09]

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