RESOLVED FIXED 217805
Open source clang warning: bitwise operation between different enumeration types [-Wenum-enum-conversion]
https://bugs.webkit.org/show_bug.cgi?id=217805
Summary Open source clang warning: bitwise operation between different enumeration ty...
David Kilzer (:ddkilzer)
Reported 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>
Attachments
Patch v1 (4.04 KB, patch)
2020-10-15 21:08 PDT, David Kilzer (:ddkilzer)
no flags
David Kilzer (:ddkilzer)
Comment 1 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)
David Kilzer (:ddkilzer)
Comment 2 2020-10-15 21:08:55 PDT
Created attachment 411530 [details] Patch v1
David Kilzer (:ddkilzer)
Comment 3 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.
Darin Adler
Comment 4 2020-10-16 09:08:02 PDT
Future direction here: Switch to using OptionSet.
David Kilzer (:ddkilzer)
Comment 5 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?
David Kilzer (:ddkilzer)
Comment 6 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.)
Darin Adler
Comment 7 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.
David Kilzer (:ddkilzer)
Comment 8 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.
Darin Adler
Comment 9 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.
Yusuke Suzuki
Comment 10 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.)
Darin Adler
Comment 11 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.
EWS
Comment 12 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].
Note You need to log in before you can comment on or make changes to this bug.