Does it test for any or all?
Created attachment 347002 [details] patch
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 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.
> 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);
(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);
> 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.
Created attachment 347019 [details] patch
Comment on attachment 347019 [details] patch Clearing flags on attachment: 347019 Committed r234811: <https://trac.webkit.org/changeset/234811>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43246242>
This broke the WinCairo builds
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.
Created attachment 347061 [details] MSVC build fix Its a build fix but I figured antti should take a look first.
Thanks for the fix!
Comment on attachment 347061 [details] MSVC build fix Clearing flags on attachment: 347061 Committed r234835: <https://trac.webkit.org/changeset/234835>