Summary: | Meaning of OptionSet::contains is unclear when used with OptionSet argument | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, benjamin, cdumez, cmarcelo, commit-queue, dbates, don.olmstead, ews-watchlist, sabouhallawa, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Antti Koivisto
2018-08-13 01:39:03 PDT
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. 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> All reviewed patches have been landed. Closing bug. |