1325351 - Maybe<T> should accept Maybe<U> in constructor and assign operator if U is convertible to T

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently, Maybe<T> accepts Maybe<U> only when T and U are both pointer, and they are convertible.

It should do a more general check so that and Maybe<U> can be assigned to Maybe<T> when U can be safely converted to T.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1325351 part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible.

Review commit: https://reviewboard.mozilla.org/r/100670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100670/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1325351 part 1 - Remove workaround for VC in Maybe.

Review commit: https://reviewboard.mozilla.org/r/100682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100682/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1325351 part 2 - Simplify 'Some' function.

Review commit: https://reviewboard.mozilla.org/r/100684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100684/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821371 [details]
Bug 1325351 part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100670/diff/1-2/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821375 [details]
Bug 1325351 part 2 - Simplify 'Some' function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100684/diff/1-2/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821371 [details]
Bug 1325351 part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100670/diff/2-3/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Try build looks good.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8821374 [details]
Bug 1325351 part 1 - Remove workaround for VC in Maybe.

https://reviewboard.mozilla.org/r/100682/#review101292

I guess bug 1052940 is related because the patched code would invoke the copy constructor, and then MSVC 2010 would instantiate the copy constructor even if the Maybe copy constructor was never called?

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8821375 [details]
Bug 1325351 part 2 - Simplify 'Some' function.

https://reviewboard.mozilla.org/r/100684/#review101294

::: mfbt/Maybe.h:441
(Diff revision 2)
> -template<typename T>
> -Maybe<typename RemoveCV<typename RemoveReference<T>::Type>::Type>
> +template<typename T,
> +         typename U = typename std::remove_cv<
> +           typename std::remove_reference<T>::type>::type>
> +Maybe<U>

I don't know if this is any more readable...

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8821371 [details]
Bug 1325351 part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible.

https://reviewboard.mozilla.org/r/100670/#review101296

If you are going to autoland this, please make sure to autoland after the 4.9 requirement has merged to autoland.  Thank you!

::: mfbt/tests/TestMaybe.cpp:923
(Diff revision 3)
> +static bool
> +TestTypeConversion()

Thank you for writing these!

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> I guess bug 1052940 is related because the patched code would invoke the
> copy constructor, and then MSVC 2010 would instantiate the copy constructor
> even if the Maybe copy constructor was never called?

I didn't check the reason... But it seems all builds are fine now without this workaround.

(In reply to Nathan Froyd [:froydnj] from comment #9)
> I don't know if this is any more readable...

It at least deduplicate the code... It becomes longer mainly because I use the std:: ones instead of mozilla:: ones.

(In reply to Nathan Froyd [:froydnj] from comment #10)
> If you are going to autoland this, please make sure to autoland after the
> 4.9 requirement has merged to autoland.  Thank you!

Based on my experience, I don't think I would be able to autoland these patches myself (which is a bug of MozReview, probably). Could you land it for me after that merges?

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

I guess that gcc 4.9 requirement has been merged. Let me just add a checkin-needed (since I cannot autoland it myself...)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4d189ca8add
part 1 - Remove workaround for VS2010 in Maybe. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/79754b9b89b7
part 2 - Simplify 'Some' function. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/45f0755ff471
part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible. r=froydnj

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for spidermonkey cgc and plain shell failure in Maybe.h:

https://hg.mozilla.org/integration/autoland/rev/38fe624589d393730a347b2b54eabce53277cf47
https://hg.mozilla.org/integration/autoland/rev/d9c389a8bcbf43f32df70eddda5d2d564cd90e12
https://hg.mozilla.org/integration/autoland/rev/330a93f6a4e82a0428544d1da45c036294c60ec0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=45f0755ff471c8204229c95627e60b55cd971748
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8436488&repo=autoland

c:\builds\moz2_slave\autoland_w32-d_sm-compacting-0\src\obj-spider\dist\include\mozilla/Maybe.h(150): error C2248: 'JS::Handle<jsid>::operator =': cannot access private member declared in class 'JS::Handle<jsid>'
c:\builds\moz2_slave\autoland_w32-d_sm-compacting-0\src\obj-spider\dist\include\js/RootingAPI.h(525): note: see declaration of 'JS::Handle<jsid>::operator ='
c:\builds\moz2_slave\autoland_w32-d_sm-compacting-0\src\obj-spider\dist\include\js/TypeDecls.h(46): note: see declaration of 'JS::Handle<jsid>'
c:\builds\moz2_slave\autoland_w32-d_sm-compacting-0\src\obj-spider\dist\include\mozilla/Maybe.h(146): note: while compiling class template member function 'mozilla::Maybe<JS::HandleId> &mozilla::Maybe<JS::HandleId>::operator =(const mozilla::Maybe<JS::HandleId> &)'
c:\builds\moz2_slave\autoland_w32-d_sm-compacting-0\src\obj-spider\dist\include\js/Proxy.h(596): note: see reference to function template instantiation 'mozilla::Maybe<JS::HandleId> &mozilla::Maybe<JS::HandleId>::operator =(const mozilla::Maybe<JS::HandleId> &)' being compiled
c:\builds\moz2_slave\autoland_w32-d_sm-compacting-0\src\obj-spider\dist\include\js/Proxy.h(580): note: see reference to class template instantiation 'mozilla::Maybe<JS::HandleId>' being compiled

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

I wonder why this doesn't happen on gecko's debug build, only cgc...

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> I wonder why this doesn't happen on gecko's debug build, only cgc...

OK, I know the reason. It is because of the presence of MOZ_EXPORT (which is "__declspec(dllexport)") on AutoEnterPolicy when the API is exported. And in that case, probably VC would create an "operator=" for it if it is dllexport.

The solution is to remove its "operator=" explicitly as well. I'll try.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> > I wonder why this doesn't happen on gecko's debug build, only cgc...
> 
> OK, I know the reason. It is because of the presence of MOZ_EXPORT (which is
> "__declspec(dllexport)") on AutoEnterPolicy when the API is exported. And in
> that case, probably VC would create an "operator=" for it if it is dllexport.
> 
> The solution is to remove its "operator=" explicitly as well. I'll try.

I guess this is probably the "bug" that comment mentions, while it is probably not. And we have better workaround for it anyway.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821374 [details]
Bug 1325351 part 1 - Remove workaround for VC in Maybe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100682/diff/1-2/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821375 [details]
Bug 1325351 part 2 - Simplify 'Some' function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100684/diff/2-3/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821371 [details]
Bug 1325351 part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100670/diff/3-4/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821374 [details]
Bug 1325351 part 1 - Remove workaround for VC in Maybe.

Since there are some additional changes, I'd like to ask for another review.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Looks all good now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b31849dde73ba36bcd62d5d6eec1b98bb04a6d8

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8821374 [details]
Bug 1325351 part 1 - Remove workaround for VC in Maybe.

https://reviewboard.mozilla.org/r/100682/#review101728

Works for me; I think I can review the js/ changes without too many problems.  r=me with change below.

::: js/public/Proxy.h:602
(Diff revision 2)
> +    // This operator needs to be deleted explicitly, otherwise Visual C++ will
> +    // create it automatically when it is part of the export JS API. In that
> +    // case, compile would fail because HandleId is not allowed to be assigned
> +    // and consequently instantiation of assign operator of mozilla::Maybe
> +    // would fail. See bug 1325351 comment 16.
> +    AutoEnterPolicy& operator=(const AutoEnterPolicy&) = delete;

It would make me feel much better if we deleted the copy constructor along with deleting the copy assignment operator.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821374 [details]
Bug 1325351 part 1 - Remove workaround for VC in Maybe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100682/diff/2-3/

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821375 [details]
Bug 1325351 part 2 - Simplify 'Some' function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100684/diff/3-4/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821371 [details]
Bug 1325351 part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100670/diff/4-5/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5917e9614e4
part 1 - Remove workaround for VC in Maybe. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/134d3abde30c
part 2 - Simplify 'Some' function. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/42fa5226e92c
part 3 - Make Maybe accept value from different Maybe type when the inner type is convertible. r=froydnj

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a5917e9614e4
https://hg.mozilla.org/mozilla-central/rev/134d3abde30c
https://hg.mozilla.org/mozilla-central/rev/42fa5226e92c