Bug 33702 - Make naming & behaviour of UString[Impl] methods more consistent.
Summary: Make naming & behaviour of UString[Impl] methods more consistent.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-14 18:22 PST by Gavin Barraclough
Modified: 2010-01-15 00:45 PST (History)
2 users (show)

See Also:


Attachments
The patch (35.32 KB, patch)
2010-01-14 18:25 PST, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-01-14 18:22:48 PST
UString::create() creates a  copy of the UChar* passed, but UStringImpl::create() assumes that it should assume ownership of the provided buffer (with UString::createNonCopying() and UStringImpl::createCopying() providing the alternate behaviours).  Unify on create() taking a copy of the provided buffer.  For non-copying cases, use the name 'adopt', and make this method take a Vector<UChar>&.  For cases where non-copying construction was being used, other than from a Vector<UChar>, change the code to allocate the storage along with the UStringImpl using UStringImpl::createUninitialized().  (The adopt() method also more closely matches that of WebCore::StringImpl).

Also, UString::createUninitialized() and UStringImpl::createUninitialized() have incompatible behaviours, in that the UString form sets the provided UChar* to a null or non-null value to indicate success or failure, but UStringImpl uses the returned PassRefPtr<UStringImpl> to indicate when allocation has failed (potentially leaving the output Char* uninitialized).  This is also incompatible with WebCore::StringImpl's behaviour, in that StringImpl::createUninitialized() will CRASH() if unable to allocate.  Some uses of createUninitialized() in JSC are unsafe, since they do not test the result for null.  UStringImpl's indication is preferable, since we may want a successful call to set the result buffer to 0 (specifically, StringImpl returns 0 for the buffer where createUninitialized() returns the empty string, which seems reasonable to catch bugs early).  UString's method cannot support UStringImpl's behaviour directly, since it returns an object rather than a pointer.
    - remove UString::createUninitialized(), replace with calls to UStringImpl::createUninitialized()
    - create a UStringImpl::tryCreateUninitialized() form UStringImpl::createUninitialized(), with current behaviour, make createUninitialized() crash on failure to allocate.
    - make cases in JSC that do not check the result call createUninitialized(), and cases that do check call tryCreateUninitialized().

Rename computedHash() to existingHash(), to bring this in line wih WebCore::StringImpl.
Comment 1 Gavin Barraclough 2010-01-14 18:25:52 PST
Created attachment 46632 [details]
The patch
Comment 2 WebKit Review Bot 2010-01-14 18:31:41 PST
Attachment 46632 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/StringPrototype.cpp:735:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/StringPrototype.cpp:772:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/UStringImpl.h:161:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3
Comment 3 Eric Seidel (no email) 2010-01-14 18:35:32 PST
Attachment 46632 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/187794
Comment 4 Gavin Barraclough 2010-01-14 22:51:40 PST
fixed in r53320
Comment 5 Eric Seidel (no email) 2010-01-15 00:28:07 PST
Looks like this broke the Windows build:
http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/9727/steps/compile-webkit/logs/stdio

At least the Debug build, or so http://build.webkit.org/console thinks.
Comment 6 Eric Seidel (no email) 2010-01-15 00:30:44 PST
I think this is the error:
WebCore_debug.lib(XMLHttpRequest.obj) : error LNK2019: unresolved external symbol "public: static class JSC::UString __cdecl JSC::UString::createNonCopying(wchar_t *,int)" (?createNonCopying@UString@JSC@@SA?AV12@PA_WH@Z) referenced in function "public: class JSC::UString __thiscall JSC::StringBuilder::release(void)" (?release@StringBuilder@JSC@@QAE?AVUString@2@XZ)

Sadly we don't have a Windows EWS bot yet.
Comment 7 Gavin Barraclough 2010-01-15 00:45:56 PST
I'm aware of this & can't see what is causing it – I think it might just be something stale on the bot, I'm going to email brian & ask him to give it a kick in the morning.

cheers,
G.