base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN.

base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

R=Nico, zea@chromium.org
BUG=557422, 554291
Committed: https://crrev.com/ca21b9b2a1ddf5ffe2a81e1d9f991ea03d89a74a
Cr-Commit-Position: refs/heads/master@{#362488}

[danakj, 2015-11-26 00:01:20]

thakis: please review base/
zea: please review sync/

[danakj, 2015-11-26 00:01:43]

Description was changed from

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN.

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

to

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN.

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

[danakj, 2015-11-26 00:01:49]

+mgiuca@chromium.org FYI

[danakj, 2015-11-26 00:02:04]

Description was changed from

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN.

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

to

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN, use std::map.

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

[danakj, 2015-11-26 00:02:15]

Description was changed from

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN, use std::map.

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

to

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

[danakj, 2015-11-26 00:04:38]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-26 00:09:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480773002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480773002/1

[commit-bot: I haz the power, 2015-11-26 01:17:18]

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

[commit-bot: I haz the power, 2015-11-26 01:17:18]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Nico, 2015-11-26 02:45:26]

The Win dbg bot complains:

FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC @obj\sync\engine\sync_core.syncer.obj.rsp /c
..\..\sync\engine\syncer.cc /Foobj\sync\engine\sync_core.syncer.obj
/Fdobj\sync\sync_core.cc.pdb 
e:\b\depot_tools\win_toolchain\vs2013_files\vc\include\utility(200) :error
C2248:
'scoped_ptr<syncer::CommitContribution,std::default_delete<T>>::scoped_ptr' :
cannot access private member declared in class
'scoped_ptr<syncer::CommitContribution,std::default_delete<T>>'
        with
        [
            T=syncer::CommitContribution
        ]
        e:\b\build\slave\win\build\src\base\memory\scoped_ptr.h(241) : see
declaration of
'scoped_ptr<syncer::CommitContribution,std::default_delete<T>>::scoped_ptr'
        with
        [
            T=syncer::CommitContribution
        ]
        This diagnostic occurred in the compiler generated function
'std::pair<const _Kty,_Ty>::pair(const std::pair<const _Kty,_Ty> &)'
        with
        [
            _Kty=syncer::ModelType
,           
_Ty=scoped_ptr<syncer::CommitContribution,std::default_delete<syncer::CommitContribution>>
        ]



I'm a bit stumped why, given that this looks very similar to many of the other
ScopedPtrMap changes we already landed. +limasdf in case they saw something like
this before.


It looks like msvc's std::utility doesn't have a "real" copy constructor, only
some templated thing, so the compiler for some weird reason decides it needs a
copy ctor, tries to generate one, and that then rightfully fails since
scoped_ptr is move-only. However, msvc's pair does have a real move ctor, so I'm
not sure why it doesn't just call that.

[limasdf, 2015-11-26 12:35:23]

limasdf@gmail.com changed reviewers:
+ limasdf@gmail.com

[limasdf, 2015-11-26 12:35:23]

I am also stuck with this
issue.(https://codereview.chromium.org/1459773002/#ps80001)

Probably MSVC2013 issue[1] is the reason.

The best idea coming up so far is holding raw pointer instead of scoped_ptr. But
in this case, we should delete pointers manually. (Sad!)

[1]:
http://stackoverflow.com/questions/28707740/error-c2248-stdpromise-typromise-...

By the way, I'm curious about why only `win_chromium_compile_dbg_ng` is failed.
Other Windows trybots don't use MSVC2013?

[Nico, 2015-11-26 13:27:44]

They all just msvs2013, but in release builds I guess some optimization
prevents the copy ctor from being needed.

Hm, bummer. I guess we should add a note about this to the status page and
we'll have to wait for 2015 for some of this.
On Nov 26, 2015 7:35 AM, <limasdf@gmail.com> wrote:

> I am also stuck with this
> issue.(https://codereview.chromium.org/1459773002/#ps80001)
>
> Probably MSVC2013 issue[1] is the reason.
>
> The best idea coming up so far is holding raw pointer instead of
> scoped_ptr. But
> in this case, we should delete pointers manually. (Sad!)
>
> [1]:
>
>
http://stackoverflow.com/questions/28707740/error-c2248-stdpromise-typromise-...
>
> By the way, I'm curious about why only `win_chromium_compile_dbg_ng` is
> failed.
> Other Windows trybots don't use MSVC2013?
>
> https://codereview.chromium.org/1480773002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-11-30 21:48:50]

On 2015/11/26 13:27:44, Nico wrote:
> They all just msvs2013, but in release builds I guess some optimization
> prevents the copy ctor from being needed.
> 
> Hm, bummer. I guess we should add a note about this to the status page and
> we'll have to wait for 2015 for some of this.
> On Nov 26, 2015 7:35 AM, <mailto:limasdf@gmail.com> wrote:
> 
> > I am also stuck with this
> > issue.(https://codereview.chromium.org/1459773002/#ps80001)
> >
> > Probably MSVC2013 issue[1] is the reason.
> >
> > The best idea coming up so far is holding raw pointer instead of
> > scoped_ptr. But
> > in this case, we should delete pointers manually. (Sad!)
> >
> > [1]:
> >
> >
>
http://stackoverflow.com/questions/28707740/error-c2248-stdpromise-typromise-...
> >
> > By the way, I'm curious about why only `win_chromium_compile_dbg_ng` is
> > failed.
> > Other Windows trybots don't use MSVC2013?
> >
> > https://codereview.chromium.org/1480773002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I think I can work around this by not constructing a pair. If I'm reading this
right, the std::pair ends up without a move constructor/assignment, and since
the pair holds a move-only type, bad things happen. So if I use operator[]
instead of insert() I can do this without a std::pair() involved. Patch sent to
the trybots.

This is awkward, I'm glad MSVC 2015 is coming soon.

[danakj, 2015-11-30 21:50:52]

Hm, maybe not..

[danakj, 2015-11-30 21:53:39]

On 2015/11/30 21:50:52, danakj (behind on reviews) wrote:
> Hm, maybe not..

Oh, typos. :D I tried to move the whole map into the value of a key.

[danakj, 2015-11-30 21:57:09]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-30 22:00:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480773002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480773002/40001

[limasdf, 2015-11-30 23:06:39]

Nice! :)

[danakj, 2015-11-30 23:08:27]

On 2015/11/30 23:06:39, limasdf wrote:
> Nice! :)

Well, it didn't work. :P

Maybe I'm misreading where the error is coming from, there's no line number for
the root cause in the code.

[commit-bot: I haz the power, 2015-11-30 23:12:38]

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

[commit-bot: I haz the power, 2015-11-30 23:12:39]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[danakj, 2015-11-30 23:20:43]

Description was changed from

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

This unconvered a use of ScopedPtrMap::Pass() though, so I just changed
it to use a std::map instead of ScopedPtrMap.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

to

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

[danakj, 2015-11-30 23:25:49]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-30 23:30:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480773002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480773002/60001

[vmpstr, 2015-11-30 23:33:34]

On 2015/11/30 23:12:39, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

I had a similar issue removing ScopedPtrVector from cc. I think it's the
exporting that causes the copy ctor to become generated or something (I'm not a
windows person). I fixed it by adding DISALLOW_COPY_AND_ASSIGN for classes that
had containers of scoped_ptrs, which made an explicit but not defined copy ctor.
In this case, it would be the Commit class I think. 

I have the trybot running here to test:
https://codereview.chromium.org/1490613002/

[chromium-reviews, 2015-11-30 23:39:53]

Commit is the one that has ContributionMap as a member, so if a copy
constructor would be generated for it, then it would cause the
ContributionMap to be copied, which would cause the members of the map to
be copied. Specifically, pairs of ModelType and
scoped_ptr<CommitContribution>, which would try and copy the scoped_ptr and
fail.

But I didn't go through that process, I just looked at the class that has a
container of scoped_ptrs as a member.

On Mon, Nov 30, 2015 at 3:36 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Mon, Nov 30, 2015 at 3:33 PM, <vmpstr@chromium.org> wrote:
>
>> On 2015/11/30 23:12:39, commit-bot: I haz the power wrote:
>>
>>> Dry run: Try jobs failed on following builders:
>>>    win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>>>
>>
>>
>>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...
>> )
>>
>> I had a similar issue removing ScopedPtrVector from cc. I think it's the
>> exporting that causes the copy ctor to become generated or something (I'm
>> not a
>> windows person). I fixed it by adding DISALLOW_COPY_AND_ASSIGN for
>> classes that
>> had containers of scoped_ptrs, which made an explicit but not defined
>> copy ctor.
>> In this case, it would be the Commit class I think.
>>
>
> Oohh interesting. The errors refer to T=CommitContribution, which is a
> pure virtual interface. Maybe it needs to go there? I'm not sure how the
> errors led you to the Commit class needing the constructor, can you explain?
>
>
>>
>> I have the trybot running here to test:
>> https://codereview.chromium.org/1490613002/
>>
>> https://codereview.chromium.org/1480773002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-11-30 23:42:30]

On Mon, Nov 30, 2015 at 3:33 PM, <vmpstr@chromium.org> wrote:

> On 2015/11/30 23:12:39, commit-bot: I haz the power wrote:
>
>> Dry run: Try jobs failed on following builders:
>>    win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>>
>
>
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...
> )
>
> I had a similar issue removing ScopedPtrVector from cc. I think it's the
> exporting that causes the copy ctor to become generated or something (I'm
> not a
> windows person). I fixed it by adding DISALLOW_COPY_AND_ASSIGN for classes
> that
> had containers of scoped_ptrs, which made an explicit but not defined copy
> ctor.
> In this case, it would be the Commit class I think.
>

Oohh interesting. The errors refer to T=CommitContribution, which is a pure
virtual interface. Maybe it needs to go there? I'm not sure how the errors
led you to the Commit class needing the constructor, can you explain?


>
> I have the trybot running here to test:
> https://codereview.chromium.org/1490613002/
>
> https://codereview.chromium.org/1480773002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-11-30 23:46:05]

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

[commit-bot: I haz the power, 2015-11-30 23:46:06]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[danakj, 2015-11-30 23:48:53]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-30 23:50:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480773002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480773002/60001

[danakj, 2015-12-01 00:37:54]

For now, I didn't convert the type to a std::map.

PTAL thakis and zea

[vmpstr, 2015-12-01 00:41:12]

On 2015/12/01 00:37:54, danakj (behind on reviews) wrote:
> For now, I didn't convert the type to a std::map.
> 
> PTAL thakis and zea

To close the loop, it does seem to fix the issue:
https://codereview.chromium.org/1490613002/ trybot is green.

[Nicolas Zea, 2015-12-01 00:46:18]

sync LGTM

[commit-bot: I haz the power, 2015-12-01 03:07:47]

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

[commit-bot: I haz the power, 2015-12-01 03:07:48]

Dry run: This issue passed the CQ dry run.

[Nico, 2015-12-01 19:27:49]

lgtm

[danakj, 2015-12-01 19:42:42]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-12-01 19:43:21]

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

[commit-bot: I haz the power, 2015-12-01 19:58:01]

Description was changed from

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

to

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

[commit-bot: I haz the power, 2015-12-01 19:58:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-01 19:59:53]

Description was changed from

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

R=Nico, zea@chromium.org
BUG=557422, 554291
==========

to

==========
base: Make ScopedPtrMap use DISALLOW_COPY_AND_ASSIGN

Currently the class uses MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
which adds a .Pass() method and works for passing with Callback. But
there are no users of this passing ownership in Callback, so switch
to the general DISALLOW_COPY_AND_ASSIGN macro.

R=Nico, zea@chromium.org
BUG=557422, 554291

Committed: https://crrev.com/ca21b9b2a1ddf5ffe2a81e1d9f991ea03d89a74a
Cr-Commit-Position: refs/heads/master@{#362488}
==========

[commit-bot: I haz the power, 2015-12-01 19:59:54]

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