Bug 149853 – Clean up Marked classes

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

[Joseph Pecoraro]

Clean up Marked classes.

Dead code, unnecessary includes, etc.

[Joseph Pecoraro]

Created attachment 262531 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 262541 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 262541 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:31:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 262541 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=262541&action=review

> Source/JavaScriptCore/heap/MarkedBlock.h:-75
> -        static const size_t atomShiftAmount = 4; // log_2(atomSize) FIXME: Change atomSize to 16.

cq- because we can also remove these unused mirrors of these shifts that I had missed in this cleanup:

llint/LowLevelInterpreter.asm
373:const AtomNumberShift = 3
374:const BitMapWordShift = 4

[Darin Adler]

Comment on attachment 262541 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=262541&action=review

> Source/JavaScriptCore/heap/MarkedSpace.cpp:30
> +struct Free : MarkedBlock::VoidFunctor {

I would have used class for this, since it has private data members.

Also, why not use functions and lambdas instead of this VoidFunctor thing?

> Source/JavaScriptCore/heap/MarkedSpace.cpp:31
> +    Free(MarkedSpace* space) : m_markedSpace(space) { }

I think this should take a reference, not a pointer.

> Source/JavaScriptCore/heap/MarkedSpace.cpp:34
>      MarkedSpace* m_markedSpace;

I think this should be a reference, not a pointer.

> Source/JavaScriptCore/heap/MarkedSpace.cpp:37
> +struct FreeOrShrink : MarkedBlock::VoidFunctor {

I would have used class for this, since it has private data members.

Also, why not use functions and lambdas instead of this VoidFunctor thing?

> Source/JavaScriptCore/heap/MarkedSpace.cpp:38
> +    FreeOrShrink(MarkedSpace* space) : m_markedSpace(space) { }

I think this should take a reference, not a pointer.

> Source/JavaScriptCore/heap/MarkedSpace.cpp:41
> +    MarkedSpace* m_markedSpace;

I think this should be a reference, not a pointer.

[Joseph Pecoraro]

> Also, why not use functions and lambdas instead of this VoidFunctor thing?

I think this is so the forEach* methods don't need multiple versions, one for a void return value (lambdas) and one where there are return values.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/190739