Summary: | Have class Exception take String by value instead of a String&& | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, darin, ews-watchlist, ryanhaddad, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=184345 | ||||||||
Attachments: |
|
Description
Daniel Bates
2018-04-06 10:31:11 PDT
Created attachment 337374 [details]
Patch
Created attachment 337375 [details]
Patch
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.
Comment on attachment 337375 [details] Patch Clearing flags on attachment: 337375 Committed r230338: <https://trac.webkit.org/changeset/230338> All reviewed patches have been landed. Closing bug. This change broke builds https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/builds/9820 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.
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. 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. |