[webkit-reviews] review requested: [Bug 49897] Get rid of the Noncopyable and FastAllocBase classes : [Attachment 75696] updated patch

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has asked  for review:
Bug 49897: Get rid of the Noncopyable and FastAllocBase classes
https://bugs.webkit.org/show_bug.cgi?id=49897

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

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #29)
> (From update of attachment 75113 [details])
> 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.

When we implemented custom allocation framework into WebKit we made a decision
to add FastAllocBase as a base class of Noncopyable because it is a superclass
and a lot of heap allocated classes are inherited from it, so some classes
needlessly got FastAllocBase's methods. In this patch I corrected these things,
because we can choose exactly which class has to be either fast allocated or
noncopyable or both. I use our static code analyzer tool (called Columbus,
check http://www.inf.u-szeged.hu/sed/softwarequality) to determine these
things.

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

Adding of WTF_MAKE_FAST_ALLOCATED to RareData struct causes compile errors on
windows:

2>c:\cygwin\home\webkit\webkit\javascriptcore\wtf\OwnPtrCommon.h(59) : error
C2248: 'JSC::CodeBlock::RareData' : cannot access private struct declared in
class 'JSC::CodeBlock'
2>	  c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(582)
: see declaration of 'JSC::CodeBlock::RareData'
2>	  c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(245)
: see declaration of 'JSC::CodeBlock'
2>	  c:\cygwin\home\webkit\WebKit\JavaScriptCore\wtf/PassOwnPtr.h(58) :
see reference to function template instantiation 'void
WTF::deleteOwnedPtr<JSC::CodeBlock::RareData>(T *)' being compiled
2>	  with
2>	  [
2>	      T=JSC::CodeBlock::RareData
2>	  ]
2>	  c:\cygwin\home\webkit\WebKit\JavaScriptCore\wtf/PassOwnPtr.h(58) :
while compiling class template member function
'WTF::PassOwnPtr<T>::~PassOwnPtr(void)'
2>	  with
2>	  [
2>	      T=JSC::CodeBlock::RareData
2>	  ]
2>	  c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(535)
: see reference to class template instantiation 'WTF::PassOwnPtr<T>' being
compiled
2>	  with
2>	  [
2>	      T=JSC::CodeBlock::RareData
2>	  ]

I don't know what causes this. Modifying its access level has solved the
problem.
 
> > 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.)

Removed.

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

FunctionParameters is a Vector and is a RefCounted. Both Vector and RefCounted
is fast allocated, so we need to add the macro to make the methods unambigous
for the compilers.

> > JavaScriptCore/pcre/pcre_exec.cpp:51
> > +#include <string.h>
> 
> Why is this getting added?

It is needed for memcpy on Chromium build.

> > JavaScriptCore/runtime/MarkStack.h:31
> > +#include <string.h>
> 
> Why did this get added?

When I removed include of Noncopyable.h memcpy missed string.h. 
 
> > JavaScriptCore/runtime/RegExpObject.h:63
> > +	     struct RegExpObjectData {
>
> What happened to the fast allocated quality here?
>

Same problem on Windows like at RareData struct. In the new patch I did the
same as with RareData.

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

Ups. It's a mistake, I removed it.
 
> > JavaScriptCore/runtime/SmallStrings.h:41
> > +	     WTF_MAKE_NONCOPYABLE(SmallStrings); WTF_MAKE_FAST_ALLOCATED;
> 
> Remove "WTF_MAKE_FAST_ALLOCATED"

Yeap, removed. SmallStringsStorage should be fast allocated not SmallStrings,
changed.

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

SharedSymbolTable is a SymbolTable which is HashMap (HashMap is fast
allocated), and it is RefCounted as well. 
The reason is same as for FunctionParameters class.

> > JavaScriptCore/wtf/MD5.h:35
> > +#include <stdint.h>
> 
> Why is this being added?

It is needed on windows because of uint8_t and etc.
MD5.cpp also needs string.h because of memcpy.

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

It doesn't need because it has it's own copy constructor: 
JavaScriptCore/wtf/OwnArrayPtr.h:50

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

Same case as in OwnArrayPtr. CC: JavaScriptCore/wtf/OwnPtr.h:55

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

Gtk build won't compile with C++ style comments. Platform.h contains only C
style comments.

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

I think it is simplier to put these to platform.h, in this way we can avoid
including Noncopyable.h and FastAllocBase.h headers.

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

This is the original Noncopyable macro implementation made by Anders Carlsson
in bug #46589.

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

Then I think it's better to putting to the end of the classes. Btw, I had a
related patch in bug #47887, but I closed it because I didn't get response and
I wanted to go ahead with this big patch.

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

That is true. I think we should wait for others opinion as well!
I haven't changed these in my new patch yet.

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

Bug #47887 was related to this.
 
> > WebKitTools/WebKitAPITest/HostWindow.h:32
> > +class HostWindow {
> 
> What happened to Noncopyable?

I did't want to include Platform.h. Corrected.

> > WebKitTools/WebKitTestRunner/TestInvocation.h:33
> > +class TestInvocation {
> 
> What happened to Noncopyable?

Ditto.
I think these are the cases which indicate separate header files for
noncopyable and fast allocated macros.