Inheriting from FastAllocBase can result in objects getting larger (bug #33896, bug #46589). Change Noncopyable and FastAllocBase inheriting to their macro implementation.
(In reply to comment #3)
> I won't update the patch until Darin's comment.
Only 3 files should be updated to make this patch appliable:
JavaScriptCore/bytecode/CodeBlock.h
WebCore/bindings/js/JSSVGContextCache.h
WebCore/svg/DeprecatedSVGAnimatedPropertyTraits.h
We would like to know what EWS bots say. ;)
It would be awesome if WTF_MAKE_FAST_ALLOCATED required a semicolon at the end of it for two reasons:
1. It just follows the typical (mental) pattern in C++ for statements (otherwise it looks like a modifier for the next item.
2. It will help with things that do regex for simple parsing. For example, editors (like emacs) that help with indenting or check-webkit-style.
A simple way to do this would be to add the following as the last line of WTF_MAKE_FAST_ALLOCATED :
typedef int ThisIsHereToForceASemicolonAfterThisMacro
(In reply to comment #24)
> It would be awesome if WTF_MAKE_FAST_ALLOCATED required a semicolon at the end of it for two reasons:
> 1. It just follows the typical (mental) pattern in C++ for statements (otherwise it looks like a modifier for the next item.
> 2. It will help with things that do regex for simple parsing. For example, editors (like emacs) that help with indenting or check-webkit-style.
>
> A simple way to do this would be to add the following as the last line of WTF_MAKE_FAST_ALLOCATED :
> typedef int ThisIsHereToForceASemicolonAfterThisMacro
Good idea, I agree with you. I modified the patch.
Comment on attachment 75113[details]
updated proposed patch rc
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?
Created attachment 75696[details]
updated patch
(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.
When I look at http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/Platform.h, it has no macros of the type that you are adding. It is solely focused on defining what platforms use, enable, and support. The added macros have nothing to do with that.
(In reply to comment #30)
> Created an attachment (id=75696) [details]
> updated patch
>
> (In reply to comment #29)
> > (From update of attachment 75113[details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75113&action=review
> >
> 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.
It would be good to try to understand why you are doing what you are doing rather than just do something to shut up a compiler.
>
> > > 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.
I guess I'll have to take this one up with him.
> >
> > 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.
Well, I'd support doing it in this patch (or I'm willing to re-visit that other patch). Hiding a change in access level makes reading code harder which is bad.
(In reply to comment #30)
> Created an attachment (id=75696) [details]
> updated patch
>
> (In reply to comment #29)
> > (From update of attachment 75113[details] [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.
Your ChangeLog has not indication of what is happening here. Also this is not reflected in the bug title.
Ideally you'd keep equivalence with respect to this aspect for this patch (since the patch is not about fixing FastAllocBase inheritance -- it is about getting rid of those classes -- and there are enough changes in here to do that without introducing a separate change for something else at the same time). Immediately after this change, get rid of the places where it isn't needed with an appropriate bug, etc.
(In reply to comment #33)
> When I look at http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/Platform.h, it has no macros of the type that you are adding. It is solely focused on defining what platforms use, enable, and support. The added macros have nothing to do with that.
Yeap, in the past we talked about to put these into platform.h. Currently, I don't know the bug's number.
> It would be good to try to understand why you are doing what you are doing rather than just do something to shut up a compiler.
Done. Patch fixed. More about the build problem: http://msdn.microsoft.com/en-us/library/tsbce2bh(VS.80).aspx> >
> > This is the original Noncopyable macro implementation made by Anders Carlsson in bug #46589.
>
> I guess I'll have to take this one up with him.
>
>
> > >
> > > 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.
>
> Well, I'd support doing it in this patch (or I'm willing to re-visit that other patch). Hiding a change in access level makes reading code harder which is bad.
I have started to put the macro's to the end of the classes. It looks terrible. Do we really want to do that?
(In reply to comment #34)
> > 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.
>
> Your ChangeLog has not indication of what is happening here. Also this is not reflected in the bug title.
Yes. What do you think about change the bug and the changelog title to Refactoring custom allocation framework and its inheritances?
> Ideally you'd keep equivalence with respect to this aspect for this patch (since the patch is not about fixing FastAllocBase inheritance -- it is about getting rid of those classes -- and there are enough changes in here to do that without introducing a separate change for something else at the same time). Immediately after this change, get rid of the places where it isn't needed with an appropriate bug, etc.
I'd like to do this one patch if it's possible. Changing from inheriting to use macros indicates extra changes than just a simple batch replace either way. Of course, I need to extend my changelog! Btw, custom allocation framework has effect only on platforms which defines ENABLE_GLOBAL_FASTMALLOC_NEW to 0.
Attachment 75808[details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808[details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808[details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808[details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061
Died at WebKitTools/Scripts/update-webkit line 132.
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 75808[details]
updated, ews test
Sorry about the style-bot noise. The stylebot has been fixed. Since this doens't pass any EWS (Except gtk) seems this is unlandable as-is.
Created attachment 77887[details]
updated, ews test, for prereview
In this patch I don't remove Noncopyable.h, but marcos are still the same with 'private:'.
Attachment 79429[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 193 in 443 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79564[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 445 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79568[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 445 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79571[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 445 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79572[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 446 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79581[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 446 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 79581[details]
proposed patch
View in context: https://bugs.webkit.org/attachment.cgi?id=79581&action=review
Zoltan fixed everything what David mentioned, except putting macros at the end of classes.
I think these macros should be at the top to get quick if a class is fast allocated or not.
Great work, r=me. Please be sure not to break any platform.
> Source/JavaScriptCore/wtf/FastAllocBase.h:60
> // Example usage:
> -// class Widget : public FastAllocBase { ... };
> +// class Widget {
> +// WTF_MAKE_FAST_ALLOCATED
> +// ...
> +// };
> +//
> +// struct Data {
> +// WTF_MAKE_FAST_ALLOCATED
> +// public:
> +// ...
> +// };
This documentation is should be enough for developers not to surprise
at private struct members if they miss to use explicit public modifier.
Additionally please write a mail to webkit-dev about this change. (and maybe a wiki page too)
2010-11-22 01:59 PST, Zoltan Horvath
2010-11-23 04:10 PST, Zoltan Horvath
2010-11-23 05:40 PST, Zoltan Horvath
2010-11-23 07:15 PST, Zoltan Horvath
2010-11-23 07:55 PST, Zoltan Horvath
2010-11-23 07:58 PST, Zoltan Horvath
2010-11-24 00:20 PST, Zoltan Horvath
2010-11-25 07:23 PST, Zoltan Horvath
2010-11-26 05:53 PST, Zoltan Horvath
2010-11-30 01:29 PST, Zoltan Horvath
2010-12-06 07:41 PST, Zoltan Horvath
2010-12-06 08:43 PST, Zoltan Horvath
2010-12-07 06:43 PST, Zoltan Horvath
2011-01-04 07:51 PST, Zoltan Horvath
2011-01-05 06:09 PST, Zoltan Horvath
2011-01-19 08:08 PST, Zoltan Horvath
2011-01-20 01:55 PST, Zoltan Horvath
2011-01-20 02:21 PST, Zoltan Horvath
2011-01-20 02:35 PST, Zoltan Horvath
2011-01-20 02:59 PST, Zoltan Horvath
2011-01-20 03:24 PST, Zoltan Horvath
2011-01-20 04:41 PST, Zoltan Horvath