Bug 154941 – Support iterating over an OptionSet and checking if it is empty

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

[Daniel Bates]

It would be convenient to be able to iterate oner the enumerators in an OptionSet and check whether an OptionSet is empty.

[Daniel Bates]

Created attachment 272708 [details]
Patch and Unit tests

[Daniel Bates]

Created attachment 272710 [details]
Patch and Unit tests

Fix typo; OptionSet::isEmpty() should have return value bool instead of StorageType.

[Radar WebKit Bug Importer]

<rdar://problem/24964187>

[Daniel Bates]

Created attachment 272785 [details]
Patch and Unit tests

Rebased patch following <http://trac.webkit.org/changeset/197523> (bug #154925).

[WebKit Commit Bot]

Attachment 272785 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/OptionSet.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/OptionSet.h:110:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 272785 [details]
Patch and Unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=272785&action=review

This looks good, but will break the WIndows build because it does not support using #include "PlatformUtilities.h".

Can you #if/def around this for Windows, and perhaps exclude the new tests that might fail without "PlatformUtiliies"?

> Tools/TestWebKitAPI/Tests/WTF/OptionSet.cpp:27
> +#include "PlatformUtilities.h"

This is causing a build failure on Windows, since it doesn't have any WK2 support features. Can we #if/def around it? What piece of PlatformUtilities.h is needed in this file?

[Daniel Bates]

(In reply to comment #6)
> [...]
> Can you #if/def around this for Windows, and perhaps exclude the new tests
> that might fail without "PlatformUtiliies"?
> 
> > Tools/TestWebKitAPI/Tests/WTF/OptionSet.cpp:27
> > +#include "PlatformUtilities.h"
> 
> This is causing a build failure on Windows, since it doesn't have any WK2
> support features. Can we #if/def around it? What piece of
> PlatformUtilities.h is needed in this file?

I will move the logic added to file PlatformUtilities.h to file Test.h.

For completeness, I added a convenience macro function EXPECT_STRONG_ENUM_EQ() to PlatformUtilities.h to support asserting a strong enum value in a test as opposed to writing the test assertions using EXPECT_TRUE()/EXPECT_FALSE(). Notice that we cannot make use of EXPECT_EQ() because it implicitly converts its arguments to integers, which will cause a compilation error for strong enums.

[Daniel Bates]

Created attachment 273266 [details]
Patch and Unit Tests

[WebKit Commit Bot]

Attachment 273266 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/OptionSet.h:109:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/OptionSet.h:110:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 273266 [details]
Patch and Unit Tests

View in context: https://bugs.webkit.org/attachment.cgi?id=273266&action=review

I think the test should cover more edge cases, such as 1 << 31 and 1 << 63.

> Source/WTF/wtf/OptionSet.h:37
> +template<typename T, typename StorageType> class OptionSetIterator;

I think it would be better for this to be a nested class template instead of a top level class template. It would be OptionSet::Iterator.

> Source/WTF/wtf/OptionSet.h:47
> +    typedef OptionSetIterator<T, StorageType> iterator;

I think in new code we should use "using" instead of typedef. Maybe we should write a style guideline.

> Source/WTF/wtf/OptionSet.h:51
> +        return OptionSet(static_cast<T>(storageType), OptionSet::FromRawValue);

Since this is inside an OptionSet member function, it can just be FromRawValue, not OptionSet::FromRawValue.

> Source/WTF/wtf/OptionSet.h:57
>      constexpr OptionSet(T t)

I wonder if this should be explicit?

> Source/WTF/wtf/OptionSet.h:71
>      OptionSet(std::initializer_list<T> initializerList)

I wonder if this should be explicit?

> Source/WTF/wtf/OptionSet.h:81
> +    constexpr bool isEmpty() const { return !m_storage; }

We’d probably like explicit operator bool. Either in addition to this or instead of it.

> Source/WTF/wtf/OptionSet.h:109
> +    OptionSetIterator(const StorageType value) : m_value(value) { }

No need for the "const" here. Ideally this would be private so that it’s only usable by OptionSet, not accidentally usable by others. Making this class template a member of OptionSet would make that work without much effort, I think. We could make this private and OptionSet would still have access to it, I believe.

> Source/WTF/wtf/OptionSet.h:116
> +    OptionSetIterator(const OptionSetIterator& other) : m_value(other.m_value) { }
> +
> +    OptionSetIterator& operator=(const OptionSetIterator& other)
> +    {
> +        m_value = other.m_value;
> +        return *this;
> +    }

This code should be omitted. They exactly match what the compiler will generate for us as long as we don’t define these explicitly.

[Daniel Bates]

(In reply to comment #10)
> Comment on attachment 273266 [details]
> Patch and Unit Tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273266&action=review
> 
> I think the test should cover more edge cases, such as 1 << 31 and 1 << 63.
> 

Will add more tests to cover these edge cases.

> > Source/WTF/wtf/OptionSet.h:37
> > +template<typename T, typename StorageType> class OptionSetIterator;
> 
> I think it would be better for this to be a nested class template instead of
> a top level class template. It would be OptionSet::Iterator.
> 

Will make this a nested class of OptionSet.

> > Source/WTF/wtf/OptionSet.h:47
> > +    typedef OptionSetIterator<T, StorageType> iterator;
> 
> I think in new code we should use "using" instead of typedef. Maybe we
> should write a style guideline.
> 

Will write using "using". We should update the WebKit Code Style Guidelines to reflect our preference for C++11 type-alias syntax instead of use of typedef.

> > Source/WTF/wtf/OptionSet.h:51
> > +        return OptionSet(static_cast<T>(storageType), OptionSet::FromRawValue);
> 
> Since this is inside an OptionSet member function, it can just be
> FromRawValue, not OptionSet::FromRawValue.
> 

Will fix.

> > Source/WTF/wtf/OptionSet.h:57
> >      constexpr OptionSet(T t)
> 
> I wonder if this should be explicit?
> 

After talking with Anders Carlsson today, we felt it was unnecessary to mark this constructor explicit as as the enum type provides sufficient type safety and allowing implicit conversion makes the syntax convenient. And OptionSet can only be instantiated for enum types.

> > Source/WTF/wtf/OptionSet.h:71
> >      OptionSet(std::initializer_list<T> initializerList)
> 
> I wonder if this should be explicit?
> 

By a similar argument we felt that it was unnecessary to make this constructor explicit.

> > Source/WTF/wtf/OptionSet.h:81
> > +    constexpr bool isEmpty() const { return !m_storage; }
> 
> We’d probably like explicit operator bool. Either in addition to this or
> instead of it.
> 

Will add explicit operator bool and keep isEmpty().

> > Source/WTF/wtf/OptionSet.h:109
> > +    OptionSetIterator(const StorageType value) : m_value(value) { }
> 
> No need for the "const" here. Ideally this would be private so that it’s
> only usable by OptionSet, not accidentally usable by others. Making this
> class template a member of OptionSet would make that work without much
> effort, I think. We could make this private and OptionSet would still have
> access to it, I believe.
> 

Will remove const, make this constructor private, and declare OptionSet as a friend of OptionSet::Iterator (since OptionSet does not have access to the private functions of OptionSet::Iterator).

> > Source/WTF/wtf/OptionSet.h:116
> > +    OptionSetIterator(const OptionSetIterator& other) : m_value(other.m_value) { }
> > +
> > +    OptionSetIterator& operator=(const OptionSetIterator& other)
> > +    {
> > +        m_value = other.m_value;
> > +        return *this;
> > +    }
> 
> This code should be omitted. They exactly match what the compiler will
> generate for us as long as we don’t define these explicitly.

Will remove this code.

[Daniel Bates]

(In reply to comment #11)
> [...]
> > > Source/WTF/wtf/OptionSet.h:81
> > > +    constexpr bool isEmpty() const { return !m_storage; }
> > 
> > We’d probably like explicit operator bool. Either in addition to this or
> > instead of it.
> > 
> 
> Will add explicit operator bool and keep isEmpty().
> 

Disregard this comment. Will not add explicitly operator bool at this time.

From talking with Anders Carlsson, we felt that that we should keep isEmpty() as OptionSet is meant to be thought of a set data structure as opposed to a bitmask. Anders also felt that it was extraneous to have both isEmpty() and an explicit operator bool. For completeness, the addition of isEmpty() and the omission of a explicit bool operator is consistent with other data structure in WTF, including HashSet and Vector.

> > > Source/WTF/wtf/OptionSet.h:109
> > > +    OptionSetIterator(const StorageType value) : m_value(value) { }
> > 
> > No need for the "const" here. Ideally this would be private so that it’s
> > only usable by OptionSet, not accidentally usable by others. Making this
> > class template a member of OptionSet would make that work without much
> > effort, I think. We could make this private and OptionSet would still have
> > access to it, I believe.
> > 
> 
> Will remove const, make this constructor private, and declare OptionSet as a
> friend of OptionSet::Iterator (since OptionSet does not have access to the
> private functions of OptionSet::Iterator).
> 
> > > Source/WTF/wtf/OptionSet.h:116
> > > +    OptionSetIterator(const OptionSetIterator& other) : m_value(other.m_value) { }
> > > +
> > > +    OptionSetIterator& operator=(const OptionSetIterator& other)
> > > +    {
> > > +        m_value = other.m_value;
> > > +        return *this;
> > > +    }
> > 
> > This code should be omitted. They exactly match what the compiler will
> > generate for us as long as we don’t define these explicitly.
> 
> Will remove this code.

[Daniel Bates]

Committed r197788: <http://trac.webkit.org/changeset/197788>