Bug 217805 - Open source clang warning: bitwise operation between different enumeration types [-Wenum-enum-conversion]
Summary: Open source clang warning: bitwise operation between different enumeration ty...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 217838
  Show dependency treegraph
 
Reported: 2020-10-15 20:30 PDT by David Kilzer (:ddkilzer)
Modified: 2020-10-16 12:44 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (4.04 KB, patch)
2020-10-15 21:08 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-10-15 20:30:45 PDT
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>
Comment 1 David Kilzer (:ddkilzer) 2020-10-15 20:37:51 PDT
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)
Comment 2 David Kilzer (:ddkilzer) 2020-10-15 21:08:55 PDT
Created attachment 411530 [details]
Patch v1
Comment 3 David Kilzer (:ddkilzer) 2020-10-15 21:10:31 PDT
(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.
Comment 4 Darin Adler 2020-10-16 09:08:02 PDT
Future direction here: Switch to using OptionSet.
Comment 5 David Kilzer (:ddkilzer) 2020-10-16 09:25:36 PDT
(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?
Comment 6 David Kilzer (:ddkilzer) 2020-10-16 09:28:29 PDT
(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.)
Comment 7 Darin Adler 2020-10-16 09:32:41 PDT
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.
Comment 8 David Kilzer (:ddkilzer) 2020-10-16 09:48:13 PDT
(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.
Comment 9 Darin Adler 2020-10-16 09:51:39 PDT
(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.
Comment 10 Yusuke Suzuki 2020-10-16 10:24:43 PDT
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.)
Comment 11 Darin Adler 2020-10-16 10:33:51 PDT
(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.
Comment 12 EWS 2020-10-16 12:44:42 PDT
Committed r268606: <https://trac.webkit.org/changeset/268606>

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