base: Remove operator& from ScopedVariant and ScopedPropVariant.

base: Remove operator& from ScopedVariant and ScopedPropVariant.

The operator& is dangerous as it makes code unclear what is happening.

For ScopedPropVariant there is already a get() method returning a
const& of the internal type. So doing &foo.get() will get a pointer to
the internal type.

For ScopedVariant, there is only operator&, so replace it with an
operator*() that returns a const&. This changes callsites from doing
something like V_FOO(&scoper) to V_FOO(&*scoper) which makes it clear
that it is getting the address of the thing inside scoper, not the
scoper itself.

R=Nico
BUG=464816
Committed: https://crrev.com/8095bc45b602f704ef85df43f9913f48fbf4cf9c
Cr-Commit-Position: refs/heads/master@{#319795}

[danakj, 2015-03-07 00:04:31]

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

[danakj, 2015-03-07 00:04:32]

+cpu cuz this is windows magic

[dmazzoni, 2015-03-09 18:14:51]

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

[dmazzoni, 2015-03-09 18:14:51]

My vote would be to add a get() accessor to ScopedVariant so I can just do
V_FOO(scoper.get()) rather than &*.

[danakj, 2015-03-09 18:27:04]

On Mon, Mar 9, 2015 at 11:14 AM, <dmazzoni@chromium.org> wrote:

> My vote would be to add a get() accessor to ScopedVariant so I can just do
> V_FOO(scoper.get()) rather than &*.
>

Normally get() would return the value as a const&, which is what
operator*() does in this patch, so you'd get V_FOO(&scoper.get()). But we
could make get() return a pointer to the internal thing instead. For a
similar class on mac, I added a get_pointer() method, but in that case it
had a get() already that returned the internal value by-value, so it was
not possible to re-use the get().

So, just double checking, but it makes sense to you to have get() return a
pointer-to-the-internal-value here?


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

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

[dmazzoni, 2015-03-09 18:27:53]

On Mon, Mar 9, 2015 at 11:18 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Mon, Mar 9, 2015 at 11:14 AM, <dmazzoni@chromium.org> wrote:
>
>> My vote would be to add a get() accessor to ScopedVariant so I can just do
>> V_FOO(scoper.get()) rather than &*.
>>
>
> Normally get() would return the value as a const&, which is what
> operator*() does in this patch, so you'd get V_FOO(&scoper.get()). But we
> could make get() return a pointer to the internal thing instead. For a
> similar class on mac, I added a get_pointer() method, but in that case it
> had a get() already that returned the internal value by-value, so it was
> not possible to re-use the get().
>

My only issue is that &* feels unnatural, since V_FOO() is one of the most
frequent ways we make use of a VARIANT, it just feels like we should be
providing an accessor that gives us the type we want so it doesn't look
like an obscure corner case.

Maybe it shouldn't be get(). How about ptr(), pointer(), or even
constptr()? I found a couple of uses of ptr(), pointer(), and Pointer()
elsewhere in the code that seem consistent with this.

Any of those would be fine with me, I just want a way to write V_FOO() that
looks deliberate.

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

[danakj, 2015-03-09 18:37:07]

On Mon, Mar 9, 2015 at 11:27 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Mon, Mar 9, 2015 at 11:18 AM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Mon, Mar 9, 2015 at 11:14 AM, <dmazzoni@chromium.org> wrote:
>>
>>> My vote would be to add a get() accessor to ScopedVariant so I can just
>>> do
>>> V_FOO(scoper.get()) rather than &*.
>>>
>>
>> Normally get() would return the value as a const&, which is what
>> operator*() does in this patch, so you'd get V_FOO(&scoper.get()). But we
>> could make get() return a pointer to the internal thing instead. For a
>> similar class on mac, I added a get_pointer() method, but in that case it
>> had a get() already that returned the internal value by-value, so it was
>> not possible to re-use the get().
>>
>
> My only issue is that &* feels unnatural, since V_FOO() is one of the most
> frequent ways we make use of a VARIANT, it just feels like we should be
> providing an accessor that gives us the type we want so it doesn't look
> like an obscure corner case.
>
> Maybe it shouldn't be get(). How about ptr(), pointer(), or even
> constptr()? I found a couple of uses of ptr(), pointer(), and Pointer()
> elsewhere in the code that seem consistent with this.
>
> Any of those would be fine with me, I just want a way to write V_FOO()
> that looks deliberate.
>

OK!

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

[danakj, 2015-03-09 18:43:53]

PTAL

[dmazzoni, 2015-03-09 20:24:12]

lgtm

[danakj, 2015-03-09 20:57:27]

On 2015/03/09 20:24:12, dmazzoni wrote:
> lgtm

Nico PTAL

[Nico, 2015-03-09 21:09:06]

win -> cpu, i thought?

[Nico, 2015-03-09 21:09:07]

win -> cpu, i thought?

[Nico, 2015-03-09 21:11:32]

but conceptually lgtm except for one thing:

https://codereview.chromium.org/985853005/diff/60001/chrome/browser/importer/...
File chrome/browser/importer/ie_importer_browsertest_win.cc (right):

https://codereview.chromium.org/985853005/diff/60001/chrome/browser/importer/...
chrome/browser/importer/ie_importer_browsertest_win.cc:197:
FAILED(property_storage->WriteMultiple(1, properties, &pv_icon.get(),
just pv_icon.get(), no &, no?

[danakj, 2015-03-09 21:44:04]

Patchset #5 (id:80001) has been deleted

[danakj, 2015-03-09 21:44:36]

On 2015/03/09 21:09:07, Nico wrote:
> win -> cpu, i thought?

Yep, wanted you to look at the base stuff :)

https://codereview.chromium.org/985853005/diff/60001/chrome/browser/importer/...
File chrome/browser/importer/ie_importer_browsertest_win.cc (right):

https://codereview.chromium.org/985853005/diff/60001/chrome/browser/importer/...
chrome/browser/importer/ie_importer_browsertest_win.cc:197:
FAILED(property_storage->WriteMultiple(1, properties, &pv_icon.get(),
On 2015/03/09 21:11:32, Nico wrote:
> just pv_icon.get(), no &, no?

Added a ptr() to ScopedPropVariant to match ScopedVariant, and used it here now.

[danakj, 2015-03-09 21:44:58]

On 2015/03/09 21:44:36, danakj wrote:
> On 2015/03/09 21:09:07, Nico wrote:
> > win -> cpu, i thought?
> 
> Yep, wanted you to look at the base stuff :)

Oh, there is base/win/OWNERS. #TIL

[cpu_(ooo_6.6-7.5), 2015-03-09 22:29:05]

lgtm

[danakj, 2015-03-09 22:29:37]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-03-09 22:29:38]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/985853005/#ps100001
(title: "scoped-operator: prop-ptr")

[commit-bot: I haz the power, 2015-03-09 22:30:23]

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

[commit-bot: I haz the power, 2015-03-10 00:41:09]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-03-10 00:41:52]

Patchset 5 (id:??) landed as
https://crrev.com/8095bc45b602f704ef85df43f9913f48fbf4cf9c
Cr-Commit-Position: refs/heads/master@{#319795}