Bug 217805 – Open source clang warning: bitwise operation between different enumeration types [-Wenum-enum-conversion]

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[David Kilzer (:ddkilzer)]

Open source clang warning: bitwise operation between different enumeration types [-Wenum-enum-conversion]

WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:876:87: warning: bitwise operation between different enumeration types ('WTF::StringImpl::StringKind' and 'WTF::StringImpl::BufferOwnership') [-Wenum-enum-conversion]
    : StringImplShape(s_refCountIncrement, length, tailPointer<UChar>(), StringNormal | BufferInternal)
                                                                         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:903:77: warning: bitwise operation between different enumeration types ('WTF::StringImpl::StringKind' and 'WTF::StringImpl::BufferOwnership') [-Wenum-enum-conversion]
    : StringImplShape(s_refCountIncrement, length, characters, StringNormal | BufferInternal)
                                                               ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:922:101: warning: bitwise operation between different enumeration types ('WTF::StringImpl::StringKind' and 'WTF::StringImpl::BufferOwnership') [-Wenum-enum-conversion]
    : StringImplShape(s_refCountIncrement, length, static_cast<const UChar*>(nullptr), StringNormal | BufferOwned)
                                                                                       ~~~~~~~~~~~~ ^ ~~~~~~~~~~~
WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:952:77: warning: bitwise operation between different enumeration types ('WTF::StringImpl::StringKind' and 'WTF::StringImpl::BufferOwnership') [-Wenum-enum-conversion]
    : StringImplShape(s_refCountIncrement, length, characters, StringNormal | BufferSubstring)
                                                               ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
WebKitBuild/Release/usr/local/include/wtf/text/StringImpl.h:1156:77: warning: bitwise operation between different enumeration types ('WTF::StringImpl::StringKind' and 'WTF::StringImpl::BufferOwnership') [-Wenum-enum-conversion]
    : StringImplShape(s_refCountIncrement, length, characters, StringSymbol | BufferSubstring)
                                                               ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~

<rdar://problem/70250742>

[David Kilzer (:ddkilzer)]

Interestingly, this constructor doesn't generate a warning, maybe because `s_hashFlag8BitBuffer` lets the compiler implicitly convert the enums to unsigned values:

inline StringImpl::StringImpl(unsigned length, Force8Bit)
    : StringImplShape(s_refCountIncrement, length, tailPointer<LChar>(), s_hashFlag8BitBuffer | StringNormal | BufferInternal)

So instead of applying static_cast<unsigned>() for these cases, we might be able to start with an unsigned value of zero as the initial bitwise value, and let the enum values be converted implicitly:

-     : StringImplShape(s_refCountIncrement, length, tailPointer<UChar>(), StringNormal | BufferInternal)
+     : StringImplShape(s_refCountIncrement, length, tailPointer<UChar>(), 0U | StringNormal | BufferInternal)

It's a bit mysterious why the "0U | " is there later, though.  Maybe we can create a constexpr value like this:

        static constexpr const unsigned s_hashZeroBaseValue = 0;

And then do this:

+     : StringImplShape(s_refCountIncrement, length, tailPointer<UChar>(), s_hashZeroBaseValue | StringNormal | BufferInternal)

[David Kilzer (:ddkilzer)]

Created attachment 411530 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #2)
> Created attachment 411530 [details]
> Patch v1

`s_hashZeroBaseValue` sounded a bit redundant, so changed to `s_hashZeroValue`.

Open to other names as well.

[Darin Adler]

Future direction here: Switch to using OptionSet.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #4)
> Future direction here: Switch to using OptionSet.

How would an OptionSet help to combine values from two different enum types?  Or you're also suggesting to combine the two enum types to a single enum type?

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> (In reply to Darin Adler from comment #4)
> > Future direction here: Switch to using OptionSet.
> 
> How would an OptionSet help to combine values from two different enum types?
> Or you're also suggesting to combine the two enum types to a single enum
> type?

(I assume there is a reason why they are separate enum types, which is why I'm asking a potentially obvious question.)

[Darin Adler]

Sorry, I was using too much shorthand. OptionSet for the options, functions to combine with the other bits, generally clean the whole thing up and have type safety.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #7)
> Sorry, I was using too much shorthand. OptionSet for the options, functions
> to combine with the other bits, generally clean the whole thing up and have
> type safety.

AFAICT, there's only one option value from each of these used in any given constructor:

    enum BufferOwnership { BufferInternal, BufferOwned, BufferSubstring, BufferExternal };

    enum StringKind {
        StringNormal = 0u, // non-symbol, non-atomic
        StringAtom = s_hashFlagStringKindIsAtom, // non-symbol, atomic
        StringSymbol = s_hashFlagStringKindIsSymbol, // symbol, non-atomic
    };

So we'd have to combine these enum types into a single enum type to use an OptionSet<>, since OptionSet<> combines multiple values from the same enum type.  Since these enum types are tracking two different concepts, I don't think it make sense to combine them just to use an OptionSet<>.

However, I do understand writing a helper function to combine these values, to keep the type safety for one more step and to reduce a bit of code duplication, though.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #8)
> AFAICT, there's only one option value from each of these used in any given
> constructor

OK, so no OptionSet.

> However, I do understand writing a helper function to combine these values,
> to keep the type safety for one more step and to reduce a bit of code
> duplication, though.

Yes, a constexpr function.

[Yusuke Suzuki]

If you use OptionSet, please check JSC JIT code too. :)
This flag fields are accessed even from JSC JIT code. So if you replace this field with OptionSet, you need to have strong bit layout compatibility with the current form (including field size, field offset etc.)

[Darin Adler]

(In reply to Yusuke Suzuki from comment #10)
> If you use OptionSet, please check JSC JIT code too. :)
> This flag fields are accessed even from JSC JIT code. So if you replace this
> field with OptionSet, you need to have strong bit layout compatibility with
> the current form (including field size, field offset etc.)

OptionSet not proposed, no bit layout changes proposed. This is all about interface to the bits, not the bits.

[EWS]

Committed r268606: <https://trac.webkit.org/changeset/268606>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411530 [details].