Bug 239481 – Use more r-value references for Text / CharacterData classes

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Use more r-value references for Text / CharacterData classes.

[Chris Dumez]

Created attachment 457853 [details]
Patch

[Chris Dumez]

Created attachment 457895 [details]
Patch

[Chris Dumez]

Created attachment 457904 [details]
Patch

[Chris Dumez]

Created attachment 457915 [details]
Patch

[Chris Dumez]

Created attachment 457935 [details]
Patch

[Sam Weinig]

Comment on attachment 457935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457935&action=review

> Source/WebCore/dom/CDATASection.h:32
> +    static Ref<CDATASection> create(Document&, String&&);

I have come to the concluding that just using "String", no "&&" is better in cases like this, and has the same behavior, because it will work for both l-values and r-values equally well. 

If this were a template function, the && would make it a universal reference, which would be even more general, but not useful for things like this.

The downside of not using "String&&" is that it is harder to find people who might be doing the wrong thing, which might be a big enough reason to keep && in these cases.

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 457935 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457935&action=review
> 
> > Source/WebCore/dom/CDATASection.h:32
> > +    static Ref<CDATASection> create(Document&, String&&);
> 
> I have come to the concluding that just using "String", no "&&" is better in
> cases like this, and has the same behavior, because it will work for both
> l-values and r-values equally well. 
> 
> If this were a template function, the && would make it a universal
> reference, which would be even more general, but not useful for things like
> this.
> 
> The downside of not using "String&&" is that it is harder to find people who
> might be doing the wrong thing, which might be a big enough reason to keep
> && in these cases.

I feel pretty strongly against using just "String" personally :/
I think it looks confusing and it makes it likely callers will fail/forget to "move" and we'll end up calling the copy constructor.

Yes, people can write efficient code with just "String" but in my opinion, they are unlikely too. "String&&" forces people to do the right thing.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Sam Weinig from comment #6)
> > Comment on attachment 457935 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457935&action=review
> > 
> > > Source/WebCore/dom/CDATASection.h:32
> > > +    static Ref<CDATASection> create(Document&, String&&);
> > 
> > I have come to the concluding that just using "String", no "&&" is better in
> > cases like this, and has the same behavior, because it will work for both
> > l-values and r-values equally well. 
> > 
> > If this were a template function, the && would make it a universal
> > reference, which would be even more general, but not useful for things like
> > this.
> > 
> > The downside of not using "String&&" is that it is harder to find people who
> > might be doing the wrong thing, which might be a big enough reason to keep
> > && in these cases.
> 
> I feel pretty strongly against using just "String" personally :/
> I think it looks confusing and it makes it likely callers will fail/forget
> to "move" and we'll end up calling the copy constructor.
> 
> Yes, people can write efficient code with just "String" but in my opinion,
> they are unlikely too. "String&&" forces people to do the right thing.

*unlikely to

[Chris Dumez]

Created attachment 457950 [details]
Patch

[Chris Dumez]

Comment on attachment 457950 [details]
Patch

Clearing flags on attachment: 457950

Committed r293059 (249790@trunk): <https://commits.webkit.org/249790@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/92002308>