WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
184360
Have class Exception take String by value instead of a String&&
https://bugs.webkit.org/show_bug.cgi?id=184360
Summary
Have class Exception take String by value instead of a String&&
Daniel Bates
Reported
2018-04-06 10:31:11 PDT
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.
Attachments
Patch
(6.57 KB, patch)
2018-04-06 10:34 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch
(6.63 KB, patch)
2018-04-06 10:37 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Bates
Comment 1
2018-04-06 10:34:34 PDT
Created
attachment 337374
[details]
Patch
Daniel Bates
Comment 2
2018-04-06 10:37:39 PDT
Created
attachment 337375
[details]
Patch
EWS Watchlist
Comment 3
2018-04-06 10:39:34 PDT
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 4
2018-04-06 10:46:02 PDT
Comment on
attachment 337375
[details]
Patch Clearing flags on attachment: 337375 Committed
r230338
: <
https://trac.webkit.org/changeset/230338
>
Daniel Bates
Comment 5
2018-04-06 10:46:04 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6
2018-04-06 10:47:33 PDT
<
rdar://problem/39242507
>
Ryan Haddad
Comment 7
2018-04-06 11:11:10 PDT
This change broke builds
https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/builds/9820
Chris Dumez
Comment 8
2018-04-06 12:19:27 PDT
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
Comment 9
2018-04-06 14:11:57 PDT
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
Comment 10
2018-04-06 14:23:15 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug