Summary: | Remove LOOSE_PASS_OWN_ARRAY_PTR from PassOwnArrayPtr.h | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, darin, dbates, mjs, mrowe, sam, sfalken | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-03-01 23:18:00 PST
Created attachment 84374 [details]
removed LOOSE_PASS_OWN_ARRAY_PTR
Comment on attachment 84374 [details] removed LOOSE_PASS_OWN_ARRAY_PTR View in context: https://bugs.webkit.org/attachment.cgi?id=84374&action=review I’m thrilled that you tackled this! > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2543 > compatibilityVersion = "Xcode 3.1"; > + developmentRegion = English; > hasScannedForEncodings = 1; Darn it, has this started happening again? I can’t remember what causes this. > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:431 > Register* registerArray = new Register[newSize]; > memcpy(registerArray + newSize - oldSize, d()->registerArray.get(), oldSize * sizeof(Register)); > - setRegisters(registerArray + newSize, registerArray, newSize); > + setRegisters(registerArray + newSize, adoptArrayPtr(registerArray), newSize); We want the adopt to be right after the "new", in the same expression if possible. So please change the local variable to an OwnArrayPtr and add the release and get calls as needed. > Source/JavaScriptCore/runtime/JSGlobalObject.h:320 > Register* registerArray = new Register[newSize]; > if (d()->registerArray) > memcpy(registerArray + count, d()->registerArray.get(), oldSize * sizeof(Register)); > - setRegisters(registerArray + newSize, registerArray, newSize); > + setRegisters(registerArray + newSize, adoptArrayPtr(registerArray), newSize); We want the adopt to be right after the "new", in the same expression if possible. So please change the local variable to an OwnArrayPtr and add the release and get calls as needed. > Source/JavaScriptCore/wtf/PassOwnArrayPtr.h:-91 > -#if !defined(LOOSE_PASS_OWN_ARRAY_PTR) || !HAVE(NULLPTR) > PassOwnArrayPtr& operator=(std::nullptr_t) { clear(); return *this; } > -#endif > + > template<typename U> PassOwnArrayPtr& operator=(const PassOwnArrayPtr<U>&); > > template<typename U> friend PassOwnArrayPtr<U> adoptArrayPtr(U*); > > -#ifdef LOOSE_PASS_OWN_ARRAY_PTR > - PassOwnArrayPtr(PtrType ptr) : m_ptr(ptr) { } > - PassOwnArrayPtr& operator=(PtrType); > -#endif > - > private: > -#ifndef LOOSE_PASS_OWN_ARRAY_PTR > explicit PassOwnArrayPtr(PtrType ptr) : m_ptr(ptr) { } > -#endif Please don’t remove the loose feature entirely; revert this part of the patch at least for now. We are using this in some non-WebKit code, and for a while we’ll need the loose mode there. It’s fine to turn the loose mode off, though. Created attachment 84517 [details]
Fixed per Darin's comment
Comment on attachment 84517 [details] Fixed per Darin's comment View in context: https://bugs.webkit.org/attachment.cgi?id=84517&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:416 > + setRegisters(registerFile.start(), PassOwnArrayPtr<Register>(), 0); In the future I think we should make nullptr work for these cases. We can’t make 0 work without allowing arbitrary pointers, but we can make nullptr work, which is much more beautiful than this. Nothing to do with this patch, but I wanted Ryosuke to know. (In reply to comment #4) > In the future I think we should make nullptr work for these cases. We can’t make 0 work without allowing arbitrary pointers, but we can make nullptr work, which is much more beautiful than this. Nothing to do with this patch, but I wanted Ryosuke to know. Yeah, I almost added a new constructor for that but I didn't since you didn't comment on it. It's probably a straight forward patch. (In reply to comment #5) > Yeah, I almost added a new constructor for that but I didn't since you didn't comment on it. It's probably a straight forward patch. There are two things we want to do with nullptr: 1) Use it to assign to an existing pointer instead of clear(). Eliminate the clear() function. 2) Use it instead of explicit calls to the empty constructor. Eliminate ugly empty constructor expressions. (In reply to comment #6) > (In reply to comment #5) > > Yeah, I almost added a new constructor for that but I didn't since you didn't comment on it. It's probably a straight forward patch. > > There are two things we want to do with nullptr: > > 1) Use it to assign to an existing pointer instead of clear(). Eliminate the clear() function. > 2) Use it instead of explicit calls to the empty constructor. Eliminate ugly empty constructor expressions. Let's just do it now. I'm sad to override your r+ but it's better to patch a cleaner code now than fixing it later. Created attachment 84674 [details]
Added PassOwnArrayPtr(std::nullptr_t)
Comment on attachment 84674 [details] Added PassOwnArrayPtr(std::nullptr_t) View in context: https://bugs.webkit.org/attachment.cgi?id=84674&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22581 > + developmentRegion = English; Is someone using old version of XCode or does XCode 4 remove this line? Comment on attachment 84674 [details] Added PassOwnArrayPtr(std::nullptr_t) View in context: https://bugs.webkit.org/attachment.cgi?id=84674&action=review Since you added the nullptr part there is a risk this won’t work with MSVC7. >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22581 >> + developmentRegion = English; > > Is someone using old version of XCode or does XCode 4 remove this line? I can’t remember! (In reply to comment #10) > (From update of attachment 84674 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84674&action=review > > Since you added the nullptr part there is a risk this won’t work with MSVC7. > > >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22581 > >> + developmentRegion = English; > > > > Is someone using old version of XCode or does XCode 4 remove this line? Yes, someone modified and committed a change to the WebCore Xcode project file using a version of Xcode < 3.2.4. > I can’t remember! From briefly searching against lists.webkit.org, you remarked that Xcode 3.2.4 adds "developmentRegion" and older versions remove it: <https://lists.webkit.org/pipermail/webkit-dev/2010-October/014651.html>. (In reply to comment #11) > (In reply to comment #10) > Yes, someone modified and committed a change to the WebCore Xcode project file using a version of Xcode < 3.2.4. And the winner is ggaren: http://trac.webkit.org/changeset/80303/trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj (In reply to comment #10) > (From update of attachment 84674 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84674&action=review > > Since you added the nullptr part there is a risk this won’t work with MSVC7. It seems like we can land this patch given win EWS didn't complain. Comment on attachment 84674 [details] Added PassOwnArrayPtr(std::nullptr_t) Clearing flags on attachment: 84674 Committed r80328: <http://trac.webkit.org/changeset/80328> All reviewed patches have been landed. Closing bug. Created attachment 84777 [details]
Patch
Patch attached to wrong bug - apologies for that. |