WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug