Bug 54029 – Give each MarkedBlock enough mark bits to cover the whole block

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Give each MarkedBlock enough mark bits to cover the whole block

[Geoffrey Garen]

Created attachment 81681 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/MarkedBlock.h:46:  BITS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/runtime/MarkedBlock.h:47:  CELLS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


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

[Darin Adler]

Comment on attachment 81681 [details]
Patch

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?

[Geoffrey Garen]

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

Indeed. I've been ignoring them for now because in the near future most of these constants will be gone.

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

I'll add a comment.

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

Fixed in final patch.

[Geoffrey Garen]

Committed r77977: <http://trac.webkit.org/changeset/77977>