[webkit-reviews] review denied: [Bug 74327] COMPILE_ASSERT in CSSStyleSelector.cpp doesn't compile on Windows : [Attachment 118935] Actually add BitfieldsType.h

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 74327: COMPILE_ASSERT in CSSStyleSelector.cpp doesn't compile on Windows
https://bugs.webkit.org/show_bug.cgi?id=74327

Attachment 118935: Actually add BitfieldsType.h
https://bugs.webkit.org/attachment.cgi?id=118935&action=review

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


> Source/JavaScriptCore/GNUmakefile.list.am:508
>	Source/JavaScriptCore/wtf/AVLTree.h \
> +	   Source/JavaScriptCore/wtf/BitfieldTypes.h \

Looks like a tabs vs. spaces mistake.

>> Source/JavaScriptCore/wtf/BitfieldTypes.h:1
>> +/*
> 
> One or more unexpected \r (^M) found; better to use only a \n 
[whitespace/carriage_return] [1]

Best way to avoid this is to set a svn:eol-style property on new files.

> Source/JavaScriptCore/wtf/BitfieldTypes.h:40
> +#if !COMPILER(MSVC)
> +typedef bool bitfieldBool;
> +#else
> +typedef unsigned bitfieldBool;
> +#endif

This is a dangerous practice. If we have an expression like this:

    m_isLoading = flags & IsLoadingFlag;

And IsLoadingFlag is 2 (or 1 << 1 or whatever), then on MSVC the code will set
m_isLoading to false, and on other compilers the code will set m_isLoading to
true. Putting this difference behind a typedef makes things worse, I think, not
better.

I don’t think a typedef is a good idea. Instead we should just use the types
signed and unsigned exclusively for bitfields on all compilers, and then audit
the code to make sure it never relies on the bitfield being boolean at the time
we change it from bool to unsigned. We can’t change from bool to unsigned all
at once without auditing the code.