[webkit-reviews] review granted: [Bug 54029] Give each MarkedBlock enough mark bits to cover the whole block : [Attachment 81681] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 54029: Give each MarkedBlock enough mark bits to cover the whole block
https://bugs.webkit.org/show_bug.cgi?id=54029

Attachment 81681: Patch
https://bugs.webkit.org/attachment.cgi?id=81681&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81681&action=review

>> Source/JavaScriptCore/runtime/MarkedBlock.h:46
>> +	const size_t BITS_PER_BLOCK = BLOCK_SIZE / CELL_SIZE;
> 
> BITS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier
names.	[readability/naming] [4]

Sure would be nice to fix these constants at some point.

> Source/JavaScriptCore/runtime/MarkedBlock.h:65
> -	   size_t cellNumber(const JSCell*);
> -	   bool isMarked(const JSCell*);
> -	   bool testAndSetMarked(const JSCell*);
> -	   void setMarked(const JSCell*);
> +	   size_t cellNumber(const void*);
> +	   bool isMarked(const void*);
> +	   bool testAndSetMarked(const void*);
> +	   void setMarked(const void*);

Why this change? Change log does not say why.

> Source/JavaScriptCore/runtime/MarkedBlock.h:112
>      inline bool MarkedBlock::isPossibleCell(const void* p)
>      {
> -	   return isCellAligned(p) && p;
> +	   return isCellAligned(p);
>      }

Do we even need an isPossibleCell function any more? Maybe calling
isCellAligned directly would be better?