1225741 - remove the explicit keyword for nsTArray copy constructor

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

+++ This bug was initially created as a clone of Bug #1156538 +++

The explicit prefix comes from:
https://hg.mozilla.org/mozilla-central/annotate/7fc4900f97cb/xpcom/glue/nsTArray.h#l1664

But we remove the explicit in this patch.

I don't understand why the explicit prefix could solve the problem at:
https://bugzilla.mozilla.org/show_bug.cgi?id=819791#c0

I have problem with the explicit and implicit prefix for nsTArray constructor.

Here is my code.
----
std::queue<nsTarray<Type>> mQueue;
mQueue.push(nsTarray<Type>());  <==== compile error here.

The problem is:
    In xcode queue impl:
    _M_push_front_aux(const value_type& __t)
    {
      value_type __t_copy = __t;   <===== the queue implementation will use implicit ctor

----
 explicit nsTArray(size_type aCapacity) : base_type(aCapacity) {}
 explicit nsTArray(const nsTArray& aOther) : base_type(aOther) {}
 MOZ_IMPLICIT nsTArray(nsTArray&& aOther) : base_type(mozilla::Move(aOther)) {}

We only have implicit ctor for move, and we need to call the implicit nsTArray(const nsTArray& aOther) here.

Could we add the implicit back?

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi botond and bz,

What do you think for comment 0?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The reason that constructor is explicit is twofold:

1)  We don't want people accidentally copying arrays.  It's an expensive operation that
    should be done explicitly.
2)  When copying, we want people to make a conscious decision about whether to do fallible
    or infallible allocation.  That also means it needs to be explicit...

In this case, the queue code is in fact making a non-obvious array copy, which should give a bit of pause about using it.  That said, if the queue code just did:

  value_type __t_copy(__t);

it would end up compiling, so I'm not sure how much this is buying us in this particular case....

I think Nathan more or less owns nsTArray now, so you should really be asking him this.

Comment 3

[Nathan Froyd [:froydnj]]

I'm not inclined to remove |explicit| just to cope with buggy code in Apple's (very old) libstdc++.  The move constructor not being explicit was OK, because having an explicit move constructor prevents you from doing the obvious thing that you have move constructors for.  In this case, the circumstances are very different.

I confess I'm not quite sure what to do as a workaround, though.  Does it work correctly to use std::queue<nsTArray<T>, std::list<nsTArray<T>> ?  That's probably less efficient, but maybe we could use the std::deque-based version on !Darwin, and the std::list version everywhere else?

Or we could write our own std::deque substitute.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Or we could write our own std::deque substitute.

We've been meaning to do this for a while anyway, to replace nsDeque.

But note that the original bug report is about std::queue, not std::deque.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > Or we could write our own std::deque substitute.
> 
> We've been meaning to do this for a while anyway, to replace nsDeque.

I will buy an unspecified amount of beverages (n > 1) for the person who consigns nsDeque to the trash bin.

> But note that the original bug report is about std::queue, not std::deque.

Indeed!  std::queue uses std::deque (or similar compatible container) under the hood (consider that the problematic function is named _M_push_front_aux, and std::queue wouldn't permit pushing on the front of it), and in this case, it's std::deque that's busted.

Comment 6

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> I'm not inclined to remove |explicit| just to cope with buggy code in
> Apple's (very old) libstdc++.

I agree that the standard library implementation is buggy in this case; my reading of the standard is that containers require their element type to be explicitly copy constructible, not implicitly.

Is the standard library version in question the one that comes with gcc 4.2? Do we have a plan for retiring support for that at some point?

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Here is my clang version. I don't know the version of standard library from this info.
g++ --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/c++/4.2.1
Apple LLVM version 7.0.0 (clang-700.1.76)
Target: x86_64-apple-darwin15.0.0
Thread model: posix

Close this bug with comment 2 and comment 3.
I will try to use another container.