Bug 26790 – ParserArenaDeletable should override delete

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kwang Yul Seo]

ParserArenaDeletable overrides new, but it does not override delete. ParserArenaDeletable must be freed by fastFree because it is allocated by fastMalloc.

[Kwang Yul Seo]

Created attachment 31998 [details]
Overeride ParserArenaDeletable delete

Overeride ParserArenaDeletable delete with fastFree.

[Eric Seidel (no email)]

Comment on attachment 31998 [details]
Overeride ParserArenaDeletable delete

Looks right to me, but this needs a ChangeLog.

http://webkit.org/coding/contributing.html

[Kwang Yul Seo]

Created attachment 32038 [details]
Overeride ParserArenaDeletable delete

ChangeLog is added

[Martin Zoubek]

*** Bug 26837 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Comment on attachment 32038 [details]
Overeride ParserArenaDeletable delete

Build fails:

In file included from JavaScriptCore/parser/Grammar.y:32,
                 from /Users/eseidel/Projects/WebKit/JavaScriptCore/AllInOneFile.cpp:59:
/Users/eseidel/Projects/WebKit/JavaScriptCore/parser/NodeConstructors.h:42: error: ‘operator delete’ must return type ‘void’

[Kwang Yul Seo]

Created attachment 32349 [details]
Overeride ParserArenaDeletable delete

Oops. Fixed a typo.

[Darin Adler]

Comment on attachment 32349 [details]
Overeride ParserArenaDeletable delete

> +        return fastFree(p);

There's no need for the return here, and it would be slightly nicer without it.

r=me

[Kwang Yul Seo]

Comment on attachment 32349 [details]
Overeride ParserArenaDeletable delete

>Index: JavaScriptCore/ChangeLog
>===================================================================
>--- JavaScriptCore/ChangeLog	(revision 45367)
>+++ JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,18 @@
>[phone]  Kwang Yul Seo  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        ParserArenaDeletable should override delete
>+        https://bugs.webkit.org/show_bug.cgi?id=26790
>+
>+        ParserArenaDeletable overrides new, but it does not override delete.
>+        ParserArenaDeletable must be freed by fastFree
>+        because it is allocated by fastMalloc.
>+
>+        * parser/NodeConstructors.h:
>+        (JSC::ParserArenaDeletable::operator delete):
>+        * parser/Nodes.h:
>+
> 2009-06-29  David Levin  <[e-mail]>
> 
>         Reviewed by Oliver Hunt.
>Index: JavaScriptCore/parser/NodeConstructors.h
>===================================================================
>--- JavaScriptCore/parser/NodeConstructors.h	(revision 45367)
>+++ JavaScriptCore/parser/NodeConstructors.h	(working copy)
>@@ -39,6 +39,11 @@ namespace JSC {
>         return fastMalloc(size);
>     }
> 
>+    inline void ParserArenaDeletable::operator delete(void* p)
>+    {
>+        fastFree(p);
>+    }
>+
>     inline ParserArenaRefCounted::ParserArenaRefCounted(JSGlobalData* globalData)
>     {
>         globalData->parser->arena().derefWithArena(adoptRef(this));
>Index: JavaScriptCore/parser/Nodes.h
>===================================================================
>--- JavaScriptCore/parser/Nodes.h	(revision 45367)
>+++ JavaScriptCore/parser/Nodes.h	(working copy)
>@@ -109,6 +109,8 @@ namespace JSC {
>         // Objects created with this version of new are not deleted when the arena is deleted.
>         // Other arrangements must be made.
>         void* operator new(size_t);
>+
>+        void operator delete(void*);
>     };
> 
>     class ParserArenaRefCounted : public RefCounted<ParserArenaRefCounted> {

[Kwang Yul Seo]

Created attachment 32457 [details]
Overeride ParserArenaDeletable delete

final.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45730.

Note: I also landed a file property change that I had sitting on my system.  This was actually the correct state for the file (which I added in a prior commit), so I'm leaving it in the archive.