Bug 188501 – Meaning of OptionSet::contains is unclear when used with OptionSet argument

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

[Antti Koivisto]

Does it test for any or all?

[Antti Koivisto]

Created attachment 347002 [details]
patch

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


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

[Said Abou-Hallawa]

Comment on attachment 347002 [details]
patch

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

> Source/WTF/ChangeLog:8
> +        The existing behavior is "contains any" but it is not very clear from the name.

I think contains() should act like containsAll(). When I say

    options1.contains(options2)

I would expect this expression to be evaluated to true if and only if "all" the bits of options2 are also in options1.

> Source/WTF/wtf/OptionSet.h:107
> +    constexpr bool containsAny(OptionSet optionSet) const
> +    {
> +        return !!(*this & optionSet);
> +    }

I do not think this is needed. The bitwise & operator overloading can do exactly what this function does. When I say

    if (option1 & options2)

The result of the expression "option1 & options2" is an OptionSet which contains option1.m_storage & option1.m_storage. The bool cast operator will check isEmpty of the the result OptionSet.

[Antti Koivisto]

> I think contains() should act like containsAll(). When I say

You may think that, but the fact the it didn't is a pretty strong indication that not everyone agrees. Better use clear names.

> I do not think this is needed. The bitwise & operator overloading can do
> exactly what this function does. When I say
> 
>     if (option1 & options2)

contains*() functions are useful because they return bools. For example 

bool foo = option1 & options2; 

doesn't compile, you need a clunky explicit conversion:

bool foo = !!(option1 & options2);

[Said Abou-Hallawa]

(In reply to Antti Koivisto from comment #4)
> > I think contains() should act like containsAll(). When I say
> 
> You may think that, but the fact the it didn't is a pretty strong indication
> that not everyone agrees. Better use clear names.
> 

The verb contains means: "have or hold (someone or something) within". Your implementation make OptionSet::contains() means "contains some of".

> > I do not think this is needed. The bitwise & operator overloading can do
> > exactly what this function does. When I say
> > 
> >     if (option1 & options2)
> 
> contains*() functions are useful because they return bools. For example 
> 
> bool foo = option1 & options2; 

How often such expression is used in our code base? I rarely see such use.

And I do not think it is good design to have two operations does the same thing.

if (option1 & options2)
if (option1.contsinas(options2))


In fact the second one will be semantically wrong if option1 is a sub set of option2 because your implementation for contains() makes it commutative operation although it should not. For example if option1 = 0x1 and option2 = 0xff

if (option1.contsinas(options2))

will be true even semantically it should be false.  In this case 

if (option1 & options2)

will be make more sense and the order of arguments will not matter.

> 
> doesn't compile, you need a clunky explicit conversion:
> 
> bool foo = !!(option1 & options2);

[Antti Koivisto]

> Your implementation make OptionSet::contains() means "contains some of".

No it doesn't.

> How often such expression is used in our code base? I rarely see such use.

A lot.

> And I do not think it is good design to have two operations does the same
> thing.

I disagree.

[Antti Koivisto]

Created attachment 347019 [details]
patch

[WebKit Commit Bot]

Comment on attachment 347019 [details]
patch

Clearing flags on attachment: 347019

Committed r234811: <https://trac.webkit.org/changeset/234811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43246242>

[Don Olmstead]

This broke the WinCairo builds

[Don Olmstead]

Ok I've had some time to narrow this down.

It was failing to compile Source/WebCore/rendering/TextDecorationPainter.cpp on MSVC within the contains method. There are multiple instances of contains with an OptionSet within that file but the part its choking on is within a lambda.

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/TextDecorationPainter.cpp#L385

I have a feeling its another MSVC bug that's the issue here. Will attempt to make a godbolt example and throw it at the Visual Studio folks.

Anyways if contains doesn't use an std::initializer_list things get back to green. Its not just WinCairo but AppleWin that are red here.

[Don Olmstead]

Created attachment 347061 [details]
MSVC build fix

Its a build fix but I figured antti should take a look first.

[Antti Koivisto]

Thanks for the fix!

[WebKit Commit Bot]

Comment on attachment 347061 [details]
MSVC build fix

Clearing flags on attachment: 347061

Committed r234835: <https://trac.webkit.org/changeset/234835>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.