Bug 188501 - Meaning of OptionSet::contains is unclear when used with OptionSet argument
Summary: Meaning of OptionSet::contains is unclear when used with OptionSet argument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-13 01:39 PDT by Antti Koivisto
Modified: 2018-08-13 22:55 PDT (History)
11 users (show)

See Also:


Attachments
patch (9.36 KB, patch)
2018-08-13 02:20 PDT, Antti Koivisto
andersca: review+
Details | Formatted Diff | Diff
patch (9.33 KB, patch)
2018-08-13 11:36 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
MSVC build fix (1.10 KB, patch)
2018-08-13 19:07 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2018-08-13 01:39:03 PDT
Does it test for any or all?
Comment 1 Antti Koivisto 2018-08-13 02:20:48 PDT
Created attachment 347002 [details]
patch
Comment 2 Build Bot 2018-08-13 02:22:46 PDT
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.
Comment 3 Said Abou-Hallawa 2018-08-13 09:10:17 PDT
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.
Comment 4 Antti Koivisto 2018-08-13 11:16:28 PDT
> 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);
Comment 5 Said Abou-Hallawa 2018-08-13 11:31:35 PDT
(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);
Comment 6 Antti Koivisto 2018-08-13 11:35:55 PDT
> 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.
Comment 7 Antti Koivisto 2018-08-13 11:36:31 PDT
Created attachment 347019 [details]
patch
Comment 8 WebKit Commit Bot 2018-08-13 12:16:17 PDT
Comment on attachment 347019 [details]
patch

Clearing flags on attachment: 347019

Committed r234811: <https://trac.webkit.org/changeset/234811>
Comment 9 WebKit Commit Bot 2018-08-13 12:16:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-08-13 12:17:19 PDT
<rdar://problem/43246242>
Comment 11 Don Olmstead 2018-08-13 14:03:10 PDT
This broke the WinCairo builds
Comment 12 Don Olmstead 2018-08-13 18:16:40 PDT
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.
Comment 13 Don Olmstead 2018-08-13 19:07:19 PDT
Created attachment 347061 [details]
MSVC build fix

Its a build fix but I figured antti should take a look first.
Comment 14 Antti Koivisto 2018-08-13 22:53:28 PDT
Thanks for the fix!
Comment 15 WebKit Commit Bot 2018-08-13 22:55:17 PDT
Comment on attachment 347061 [details]
MSVC build fix

Clearing flags on attachment: 347061

Committed r234835: <https://trac.webkit.org/changeset/234835>
Comment 16 WebKit Commit Bot 2018-08-13 22:55:19 PDT
All reviewed patches have been landed.  Closing bug.