Bug 55554

Summary: Remove LOOSE_PASS_OWN_ARRAY_PTR from PassOwnArrayPtr.h
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Web Template FrameworkAssignee: 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 Flags
removed LOOSE_PASS_OWN_ARRAY_PTR
none
Fixed per Darin's comment
none
Added PassOwnArrayPtr(std::nullptr_t)
none
Patch mrowe: review-

Description Ryosuke Niwa 2011-03-01 23:18:00 PST
We should remove LOOSE_PASS_OWN_ARRAY_PTR from PassOwnArrayPtr.h
Comment 1 Ryosuke Niwa 2011-03-01 23:37:00 PST
Created attachment 84374 [details]
removed LOOSE_PASS_OWN_ARRAY_PTR
Comment 2 Darin Adler 2011-03-02 17:17:46 PST
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.
Comment 3 Ryosuke Niwa 2011-03-02 21:37:31 PST
Created attachment 84517 [details]
Fixed per Darin's comment
Comment 4 Darin Adler 2011-03-03 08:21:32 PST
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.
Comment 5 Ryosuke Niwa 2011-03-03 14:51:22 PST
(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.
Comment 6 Darin Adler 2011-03-03 16:35:59 PST
(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.
Comment 7 Ryosuke Niwa 2011-03-03 18:12:00 PST
(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.
Comment 8 Ryosuke Niwa 2011-03-03 18:19:09 PST
Created attachment 84674 [details]
Added PassOwnArrayPtr(std::nullptr_t)
Comment 9 Ryosuke Niwa 2011-03-03 18:20:12 PST
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 10 Darin Adler 2011-03-03 18:33:32 PST
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!
Comment 11 Daniel Bates 2011-03-03 19:01:29 PST
(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>.
Comment 12 Ryosuke Niwa 2011-03-03 19:06:45 PST
(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
Comment 13 Ryosuke Niwa 2011-03-03 22:21:06 PST
(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 14 Ryosuke Niwa 2011-03-03 22:25:59 PST
Comment on attachment 84674 [details]
Added PassOwnArrayPtr(std::nullptr_t)

Clearing flags on attachment: 84674

Committed r80328: <http://trac.webkit.org/changeset/80328>
Comment 15 Ryosuke Niwa 2011-03-03 22:26:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Steve Falkenburg 2011-03-04 10:19:54 PST
Created attachment 84777 [details]
Patch
Comment 17 Steve Falkenburg 2011-03-04 10:24:51 PST
Patch attached to wrong bug - apologies for that.