make nsCSSKeyframeRule constructor's aKeys argument an rvalue reference

RESOLVED FIXED in Firefox 45

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: heycam, Assigned: dbaron)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
There's a warning above the nsCSSKeyframeConstructor pointing out that it will steal the contents of the aKeys array argument.  We should just make this an rvalue reference so that the caller has to explicitly Move() the array in.
Assignee: nobody → dbaron
Created attachment 8690253 [details] [diff] [review]
Use rvalue-reference and Move rather than just comments to show behavior of nsCSSKeyframeRule constructor
Attachment #8690253 - Flags: review?(quanxunzhen)
Comment on attachment 8690253 [details] [diff] [review]
Use rvalue-reference and Move rather than just comments to show behavior of nsCSSKeyframeRule constructor

Review of attachment 8690253 [details] [diff] [review]:
-----------------------------------------------------------------

This change looks good to me, with some suggestions:

::: layout/style/nsCSSParser.cpp
@@ +4261,5 @@
>    }
>  
>    // Takes ownership of declaration, and steals contents of selectorList.
>    RefPtr<nsCSSKeyframeRule> rule =
> +    new nsCSSKeyframeRule(Move(selectorList), declaration, linenum, colnum);

With this change, the second part of the comment seems to be redundant here. I think we can remove that.

I suggest we also make aDeclaration accept an already_AddRef<Declaration>&& and use declaration.forget() here, which makes the comment completely redundant, and also reduce a pair of meaningless AddRef()/Release() calls.
Attachment #8690253 - Flags: review?(quanxunzhen) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> I suggest we also make aDeclaration accept an already_AddRef<Declaration>&&
> and use declaration.forget() here, which makes the comment completely
> redundant, and also reduce a pair of meaningless AddRef()/Release() calls.

I'm curious what the reason is to prefer already_AddRefed<Declaration>&& over already_AddRefed<Declaration> (without &&).
Flags: needinfo?(quanxunzhen)
Created attachment 8690361 [details] [diff] [review]
patch 2 - Use already_AddRefed<Declaration>&& as parameter to nsCSSKeyframeRule constructor to avoid extra reference count cycle
Attachment #8690361 - Flags: review?(quanxunzhen)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #3)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> > I suggest we also make aDeclaration accept an already_AddRef<Declaration>&&
> > and use declaration.forget() here, which makes the comment completely
> > redundant, and also reduce a pair of meaningless AddRef()/Release() calls.
> 
> I'm curious what the reason is to prefer already_AddRefed<Declaration>&&
> over already_AddRefed<Declaration> (without &&).

Basically you cannot use this form here. It just won't compile.

The reason is that, the value returned from a function is a pure rvalue, which can be referenced by only "const type&" and "type&&" in parameter. In normal cases, declaring "type" without any "&" works because there is an implicit conversion via copy constructor, which accepts "const type&" and copy the content to the new object for the argument. However, already_AddRefed has no copy constructor, so the implicit conversion there won't work.
Flags: needinfo?(quanxunzhen)
Comment on attachment 8690361 [details] [diff] [review]
patch 2 - Use already_AddRefed<Declaration>&& as parameter to nsCSSKeyframeRule constructor to avoid extra reference count cycle

Review of attachment 8690361 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

I really think the comments in both files are redundant, because "forget" and "move" have indicated the movement of the ownership. (And we want to add a check in our clang plugin to ensure that variables are not used after they are forgotten or moved, see bug 1186706.) But if you feel the comments do help making the code clearer, that's probably also fine.
Attachment #8690361 - Flags: review?(quanxunzhen) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #3)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> > > I suggest we also make aDeclaration accept an already_AddRef<Declaration>&&
> > > and use declaration.forget() here, which makes the comment completely
> > > redundant, and also reduce a pair of meaningless AddRef()/Release() calls.
> > 
> > I'm curious what the reason is to prefer already_AddRefed<Declaration>&&
> > over already_AddRefed<Declaration> (without &&).
> 
> Basically you cannot use this form here. It just won't compile.

Ouch... It works, the implicit conversion works via the move constructor path as well.

OK, I don't know. Using && here could eliminate one move constructor call, but that probably doesn't make much sense because compilers should be able to optimize that out anyway.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3a04a4d334f383b0a2c2e0a4b3b1540865de7e
Bug 1221823 patch 1 - Use rvalue-reference and Move rather than just comments to show behavior of nsCSSKeyframeRule constructor.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f8ba112b5dfb79f954a4d917523a893948edbf
Bug 1221823 patch 2 - Use already_AddRefed<Declaration>&& as parameter to nsCSSKeyframeRule constructor to avoid extra reference count cycle.  r=xidorn

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d3a04a4d334
https://hg.mozilla.org/mozilla-central/rev/69f8ba112b5d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
So there is effectively no difference between using value or rvalue reference in parameter for already_AddRefed or any other move-only type (e.g. UniquePtr) in practice. Thus, it is more a code style question.

For me, using rvalue reference makes it clearer that it intends to accept a moved value. In a speech on CppCon 2014, Herb Sutter also suggested for "in & move from" case, "f(X&&)" can be used for all types. [1] And a friend, who follows the C++ committee closely, suggested that, value type should be used only if the function wants a disjointed object. [2]

So I think it is reasonable here to prefer "already_AddRefed<>&&" over "already_AddRefed<>". (And also, prefer "UniquePtr<>&&" in parameters over "UniquePtr<>".)


[1] https://youtu.be/xnqTKD8uD64?t=59m11s
[2] https://twitter.com/lichray/status/700923997534334976 (Chinese)
You need to log in before you can comment on or make changes to this bug.