[Webkit-unassigned] [Bug 213383] -Wsign-compare in isValidOptionSet

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=213383

--- Comment #4 from Michael Catanzaro <mcatanzaro at gnome.org> ---
OK, I present two... options. ;)

Option #1: OptionSetValueChecker should match the OptionSet's StorageType, which is std::make_unsigned_t<std::underlying_type_t<E>>, instead of directly using std::underlying_type_t<E>:

diff --git a/Source/WTF/wtf/OptionSet.h b/Source/WTF/wtf/OptionSet.h
index de111b16b355..8450b1a240d9 100644
--- a/Source/WTF/wtf/OptionSet.h
+++ b/Source/WTF/wtf/OptionSet.h
@@ -253,8 +253,8 @@ private:
 template<typename E>
 WARN_UNUSED_RETURN constexpr bool isValidOptionSet(OptionSet<E> optionSet)
 {
-    auto allValidBitsValue = OptionSetValueChecker<std::underlying_type_t<E>, typename EnumTraits<E>::values>::allValidBits();
-    return (optionSet.toRaw() | allValidBitsValue) == allValidBitsValue;
+    auto allValidBitsValue = OptionSetValueChecker<std::make_unsigned_t<std::underlying_type_t<E>>, typename EnumTraits<E>::values>::allValidBits();
+    return (optionSet.toRaw() | allValidBitsValue) == std::underlying_type_t<E>(allValidBitsValue);
 }

That fixes the warning without making any changes in other files. I'll attach this patch for review.

Option #2: we could require the OptionSet to use an unsigned underlying value to call isValidOptionSet using std::enable_if. That would enforce unsigned underlying value only for OptionSets that are validated by isValidOptionSet. We only have to change the underlying type of WebKit::InputMethodState::Hint in Source/WebKit/Shared/glib/InputMethodState.h. I decided against making this my preferred option because valid OptionSets do not actually have to use unsigned underlying storage, so it would be a bit weird to require that to call isValidOptionSet, but this would be useful if we want to enforce this for types sent over IPC. (Do we want to enforce it for types sent over IPC?)

Option #3: would be to prohibit OptionSet from ever using signed underlying value, but that's going to require changing a lot of enums. Doesn't seem worth it, so I don't propose this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200619/e520d213/attachment.htm>