Bug 184360 – Have class Exception take String by value instead of a String&&

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

The requirement of class Exception to take its optional message argument as String&& instead of String came up in bug 184345, comment 13. This requirement means that a caller must explicitly invoke the String constructor on a lvalue String object in order to instantiate an Exception. We should support instantiating an Exception with either an lvalue or rvalue String for convenience.

Although it can be argued that having Exception take a String by value instead of String&& can lead to missed opportunities to WTFMove() a String object into Exception such mistakes are just that, missed opportunities. That is, correctness is not affected and we may perform an unnecessary ref/deref of the underlying StringImpl when instantiating an Exception. If such missed opportunities show up in profiles and such mistakes happen often then we can re-evaluate the decision to have Exception take a String by value.

[Daniel Bates]

Created attachment 337374 [details]
Patch

[Daniel Bates]

Created attachment 337375 [details]
Patch

[EWS Watchlist]

Attachment 337375 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Exception.h:56:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 337375 [details]
Patch

Clearing flags on attachment: 337375

Committed r230338: <https://trac.webkit.org/changeset/230338>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39242507>

[Ryan Haddad]

This change broke builds https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/builds/9820

[Chris Dumez]

Comment on attachment 337375 [details]
Patch

This is pretty recent code from Darin so I added him in cc.

I personally prefer String&& it forces people to write efficient code.

[Darin Adler]

I don’t feel strongly about this either way. I chose String&& for roughly the same reason as Chris: since this was a brand new class I figured I would bias towards the more efficient move semantic.

[Darin Adler]

Some more thoughts on this.

(In reply to Daniel Bates from comment #0)
> That is, correctness is not affected and we may perform an unnecessary
> ref/deref of the underlying StringImpl when instantiating an Exception. If
> such missed opportunities show up in profiles and such mistakes happen often
> then we can re-evaluate the decision to have Exception take a String by
> value.

Since String is simply an object that wraps a RefPtr, to think this through we need to basically make an argument about all functions that take ownership of a ref to a pointed-to object. If the type involved is T, which of these should the function take (ignoring nullability, which brings in three more types)?

    T*
    RefPtr<T>
    RefPtr<T>&&

A downside of T* is that there is no way to optimize the transfer of ownership. So it leads to maximum reference count churn. But it’s the most flexible in terms of "the easiest type to pass"; always just a get() or a ptr() away.

A downside of RefPtr<T> is that extra reference count churn is relatively common; any time a RefPtr from a local variable is used and someone forgets to use WTFMove, it will occur.

A downside of RefPtr<T>&& is that callers have to write out type casts when they want to share rather than moving. It is this downside that caused Dan to propose this change. But these cases should be really rare. For String, it’s basically only when we are dealing with functions that return const String& or when we want to have an implicit conversion to String.

I don’t want to do something different for String than we would for RefPtr<StringImpl>. In fact, I dream about some day making the RefPtr aspect of String more explicit for callers and making it more straightforward to use StringImpl* and StringImpl& as appropriate. Some day maybe StringImpl can be renamed to String.