[webkit-reviews] review denied: [Bug 49897] Get rid of the Noncopyable and FastAllocBase classes : [Attachment 75113] updated proposed patch rc

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 49897: Get rid of the Noncopyable and FastAllocBase classes
https://bugs.webkit.org/show_bug.cgi?id=49897

Attachment 75113: updated proposed patch rc
https://bugs.webkit.org/attachment.cgi?id=75113&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75113&action=review

It isn't clear why the noncopyable attribute wasn't retained in various places.


It also isn't clear why noncopyable was converted to noncopyable+fastallocated
in some places but only noncopyable in others.

Lastly, I have some concerns about the macro placement and about some side
effects of what is hidden in those macros (private:/public: , etc).

> JavaScriptCore/bytecode/CodeBlock.h:582
> +    public:

Remove "public:" here. (Not equivalent to what was here before.)

> JavaScriptCore/bytecode/CodeBlock.h:606
> +    private:

Remove "private:" here.

> JavaScriptCore/parser/Nodes.h:116
> +	   WTF_MAKE_FAST_ALLOCATED;

Remove "WTF_MAKE_FAST_ALLOCATED" (because it appears to me that this is not
equivalent to what was here before.)

> JavaScriptCore/parser/Nodes.h:1476
> +	   WTF_MAKE_FAST_ALLOCATED;

Remove "WTF_MAKE_FAST_ALLOCATED"

> JavaScriptCore/pcre/pcre_exec.cpp:51
> +#include <string.h>

Why is this getting added?

> JavaScriptCore/runtime/MarkStack.h:31
> +#include <string.h>

Why did this get added?

> JavaScriptCore/runtime/RegExpObject.h:63
> +	   struct RegExpObjectData {

What happened to the fast allocated quality here?

> JavaScriptCore/runtime/RegExpObject.h:64
> +	   public:

Remove "public:"

> JavaScriptCore/runtime/SmallStrings.h:41
> +	   WTF_MAKE_NONCOPYABLE(SmallStrings); WTF_MAKE_FAST_ALLOCATED;

Remove "WTF_MAKE_FAST_ALLOCATED"

> JavaScriptCore/runtime/SymbolTable.h:125
> +	   WTF_MAKE_FAST_ALLOCATED;

Remove "WTF_MAKE_FAST_ALLOCATED"

> JavaScriptCore/wtf/MD5.h:35
> +#include <stdint.h>

Why is this being added?

> JavaScriptCore/wtf/OwnArrayPtr.h:37
> +template <typename T> class OwnArrayPtr {

Needs  WTF_MAKE_NONCOPYABLE

> JavaScriptCore/wtf/OwnPtr.h:41
> +    template<typename T> class OwnPtr {

Needs  WTF_MAKE_NONCOPYABLE

> JavaScriptCore/wtf/Platform.h:1122
> +/* Noncopyable's implementation as a macro */

Use C++ style comments.

platform.h seems like the wrong header file for this.  Why not put them in the
header files where they were?

> JavaScriptCore/wtf/Platform.h:1133
> +	   private: \

Hiding private: inside of this macro is unexpected and will lead to people
scratching their heads when their class members become private after this is
put in. (Much like hiding goto or return in a macro).

Instead how about just putting this macro at the end of classes and explicitly
adding a private: section if there isn't one.

> JavaScriptCore/wtf/Platform.h:1147
> +public: \

Same comment as above but instead of putting this macro at the end of the
class, one could put it at the end of the public section.

I tend to expect important information at the top of the class and whether the
class is fast allocated or not really doesn't matter to me when I trying to use
it.

> JavaScriptCore/wtf/Platform.h:1176
> +private: \

Ditto.

> WebKitTools/WebKitAPITest/HostWindow.h:32
> +class HostWindow {

What happened to Noncopyable?

> WebKitTools/WebKitTestRunner/TestInvocation.h:33
> +class TestInvocation {

What happened to Noncopyable?