Fix memory leak in assert scopes.

Fix memory leak in assert scopes.

R=svenpanne@chromium.org
BUG=246567
Committed: https://code.google.com/p/v8/source/detail?r=15106

[Sven Panne, 2013-06-12 06:57:36]

DBC...

https://chromiumcodereview.appspot.com/15709020/diff/1/src/assert-scope.h
File src/assert-scope.h (right):

https://chromiumcodereview.appspot.com/15709020/diff/1/src/assert-scope.h#new...
src/assert-scope.h:81: enum AcquireAssertDataMode { CREATE_IF_ABSENT,
NULL_IF_ABSENT };
This is getting a little bit confusing... I think that instead of introducing
this enum, we should actually just split AssertData() into 2 methods: One for
retrieving PerThreadAssertData from TLS and one putting it into TLS. IsAllowed
simply uses the first method, while the PerThreadAssertScopeBase constructor
uses a combination of the 2 methods.

http://www.refactoring.com/catalog/replaceParameterWithExplicitMethods.html

[Yang, 2013-06-12 08:23:59]

On 2013/06/12 06:57:36, Sven Panne wrote:
> DBC...
> 
> https://chromiumcodereview.appspot.com/15709020/diff/1/src/assert-scope.h
> File src/assert-scope.h (right):
> 
>
https://chromiumcodereview.appspot.com/15709020/diff/1/src/assert-scope.h#new...
> src/assert-scope.h:81: enum AcquireAssertDataMode { CREATE_IF_ABSENT,
> NULL_IF_ABSENT };
> This is getting a little bit confusing... I think that instead of introducing
> this enum, we should actually just split AssertData() into 2 methods: One for
> retrieving PerThreadAssertData from TLS and one putting it into TLS. IsAllowed
> simply uses the first method, while the PerThreadAssertScopeBase constructor
> uses a combination of the 2 methods.
> 
> http://www.refactoring.com/catalog/replaceParameterWithExplicitMethods.html

Changed as suggested. Please take another look.

[Sven Panne, 2013-06-12 09:41:16]

https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h
File src/assert-scope.h (right):

https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h#...
src/assert-scope.h:81: enum AcquireAssertDataMode { CREATE_IF_ABSENT,
NULL_IF_ABSENT };
Nuke this.

https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h#...
src/assert-scope.h:87: Thread::SetThreadLocal(thread_local_key, data_);
Put the TLS write into a private method and use it here...

https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h#...
src/assert-scope.h:98: Thread::SetThreadLocal(thread_local_key, NULL);
... and here.

[Yang, 2013-06-12 15:26:31]

On 2013/06/12 09:41:16, Sven Panne wrote:
> https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h
> File src/assert-scope.h (right):
> 
>
https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h#...
> src/assert-scope.h:81: enum AcquireAssertDataMode { CREATE_IF_ABSENT,
> NULL_IF_ABSENT };
> Nuke this.
> 
>
https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h#...
> src/assert-scope.h:87: Thread::SetThreadLocal(thread_local_key, data_);
> Put the TLS write into a private method and use it here...
> 
>
https://chromiumcodereview.appspot.com/15709020/diff/5001/src/assert-scope.h#...
> src/assert-scope.h:98: Thread::SetThreadLocal(thread_local_key, NULL);
> ... and here.

I addressed the comments.

[Sven Panne, 2013-06-13 06:11:38]

LGTM with a nit

https://codereview.chromium.org/15709020/diff/10001/src/assert-scope.h
File src/assert-scope.h (right):

https://codereview.chromium.org/15709020/diff/10001/src/assert-scope.h#newcod...
src/assert-scope.h:109: void SetThreadLocalData(PerThreadAssertData* data) {
nit: add "static"

[Yang, 2013-06-13 07:47:48]

Committed patchset #3 manually as r15106 (presubmit successful).