Summary: | Clean up Marked classes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | JavaScriptCore | Assignee: | Joseph Pecoraro <joepeck> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue, fpizlo, ggaren, joepeck, kling | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2015-10-06 11:38:41 PDT
Created attachment 262531 [details]
[PATCH] Proposed Fix
Created attachment 262541 [details]
[PATCH] Proposed Fix
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.
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 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. > 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.
|