Bug 55554 – Remove LOOSE_PASS_OWN_ARRAY_PTR from PassOwnArrayPtr.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Ryosuke Niwa]

We should remove LOOSE_PASS_OWN_ARRAY_PTR from PassOwnArrayPtr.h

[Ryosuke Niwa]

Created attachment 84374 [details]
removed LOOSE_PASS_OWN_ARRAY_PTR

[Darin Adler]

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.

[Ryosuke Niwa]

Created attachment 84517 [details]
Fixed per Darin's comment

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Darin Adler]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 84674 [details]
Added PassOwnArrayPtr(std::nullptr_t)

[Ryosuke Niwa]

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?

[Darin Adler]

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!

[Daniel Bates]

(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>.

[Ryosuke Niwa]

(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

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Comment on attachment 84674 [details]
Added PassOwnArrayPtr(std::nullptr_t)

Clearing flags on attachment: 84674

Committed r80328: <http://trac.webkit.org/changeset/80328>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Steve Falkenburg]

Created attachment 84777 [details]
Patch

[Steve Falkenburg]

Patch attached to wrong bug - apologies for that.