Closed Bug 1119080 Opened 5 years ago Closed 5 years ago

already_AddRefed in AlreadyAddRefed.h is not compatible with VS2015: error C2280: attempting to reference a deleted function

Categories

(Core :: MFBT, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file Build log
No description provided.
This is sort of a bizarre error.
The MSVC 2015 behavior may not be as crazy as it seems at first.

It seems like all the instances where the compiler complains are due to this scenerio:


already_AddRefed<Base>
foo()
{
   nsCOMPtr<Derived> subclass = ...;
   return subclass.forget();
}

nsCOMPtr<Derived>::forget() will return already_AddRefed<Derived>. Since we need to return already_AddRefed<Base>, we need to do a conversion. This conversion seems to confuse MSVC 2015's copy elision logic, so it doesn't do copy elision (??? not sure ???). Then, it feels like it needs to use already_Addrefed<T> copy constructor, which has been deleted.

As far as I understand the specification, the current definition of already_Addrefed<T> is not guaranteed to work according to the spec, because copy elision is always optional, and if the compiler doesn't do copy elision, then it needs a copy constructor.

By adding an explicit nsRefPtr<Base>/nsCOMPtr<Base> conversion, I can work around the error. It isn't clear how many instances of this pattern there are in the code base, so before I continue, I'd like to hear what you think. In particular, is there a better workaround I should attempt?
Attachment #8547127 - Flags: feedback?(jwalden+bmo)
Attachment #8547127 - Flags: feedback?(ehsan)
The problem may be caused by the fact that the conversion operator is non-const.

non-const conversion operators violate the principle of least surprise, because we don't expect static_cast<Base>(x) to modify x. In particular, consider the following code:

   already_Addrefed<Derived> foo = GetFoo();
   already_Addrefed<Base> base(foo);
   foo->doSomething(); // NULL POINTER DEREFERENCE!!!

So it's a good idea to avoid such non-const conversion operators anyway, regardless of the VS2015 issue.

Here is my attempt to do so. Note that the above-cited bad code sample won't compile, because of the missing std::move:

   already_Addrefed<Derived> foo = GetFoo();
   already_Addrefed<Base> base(std::move(foo));
   // NULL POINTER DEREFERENCE, but you expect that after
   // std::move! (This is why std::move exists, more or less.)
   foo->doSomething();

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86b1d33f4f68
Assignee: nobody → brian
Attachment #8547127 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8547127 - Flags: feedback?(jwalden+bmo)
Attachment #8547127 - Flags: feedback?(ehsan)
Flags: needinfo?(brian)
Attachment #8547902 - Flags: review?(jwalden+bmo)
Target Milestone: --- → mozilla38
Attachment #8547902 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/9f889f63ea6c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.