CSS serialization expects comments between certain tokens
Created attachment 409632 [details] Patch
Created attachment 409634 [details] Patch
Created attachment 409899 [details] Patch
Created attachment 409903 [details] Patch
Comment on attachment 409903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409903&action=review > Source/WebCore/css/parser/CSSParserToken.cpp:489 > + // Weirdly Clang errors if you try to use the fold expression in buildNextTokenNeedsCommentTable() because the true value is unused. > + // So we just build the table by hand here instead. Before landing I'll amend this with a link to the radar against Clang for the warning: rdar://69710661
Comment on attachment 409903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409903&action=review > LayoutTests/imported/w3c/web-platform-tests/css/css-syntax/serialize-consecutive-tokens-expected.txt:2 > +PASS Serialization of consecutive foo and bar tokens. Looks like my "strip trailing spaces" change came after you generated this file. Need to strip the trailing spaces.
Created attachment 409909 [details] Patch
Comment on attachment 409903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409903&action=review >> LayoutTests/imported/w3c/web-platform-tests/css/css-syntax/serialize-consecutive-tokens-expected.txt:2 >> +PASS Serialization of consecutive foo and bar tokens. > > Looks like my "strip trailing spaces" change came after you generated this file. Need to strip the trailing spaces. Yeah, I just uploaded a new patch now. I'm double checking locally that I have the right number of new lines at the end of the file though.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review > Source/WebCore/ChangeLog:12 > + don't understand, the spec requires a comment rather than > + whitespace so that's what this patch does. Since there are 32 Please provide spec links. > LayoutTests/imported/w3c/web-platform-tests/css/selectors/anplusb-selector-parsing-expected.txt:102 > +FAIL :nth-last-of-type(1n+0) should be parsed and serialized correctly assert_equals: expected ":nth-last-of-type(n)" but got ":nth-last-of-type(1n/**/+0)" > +FAIL :nth-last-of-type(n+0) should be parsed and serialized correctly assert_equals: expected ":nth-last-of-type(n)" but got ":nth-last-of-type(n/**/+0)" This is very surprising.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >> Source/WebCore/ChangeLog:12 >> + whitespace so that's what this patch does. Since there are 32 > > Please provide spec links. Done. >> LayoutTests/imported/w3c/web-platform-tests/css/selectors/anplusb-selector-parsing-expected.txt:102 >> +FAIL :nth-last-of-type(n+0) should be parsed and serialized correctly assert_equals: expected ":nth-last-of-type(n)" but got ":nth-last-of-type(n/**/+0)" > > This is very surprising. This happens because we incorrectly parse the an-plus-b syntax as a Dimension token followed by a number token. According to the table https://drafts.csswg.org/css-syntax/#serialization here, this should be serialized as the test is outputting. I plan on fixing our parsing of this syntax to the correct dimension token followed by delimiter then a number, which should fix this issue, in my next patch.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review > Source/WebCore/css/parser/CSSParserToken.cpp:429 > + constexpr auto nextTokenNeedsCommentTable = buildNextTokenNeedsCommentTable(IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken); > + appendCommentIfNeeded(nextTokenNeedsCommentTable, '-'); Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this: appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken }); We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >> Source/WebCore/css/parser/CSSParserToken.cpp:429 >> + appendCommentIfNeeded(nextTokenNeedsCommentTable, '-'); > > Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this: > > appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken }); > > We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list. Sure, that makes sense.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >>> Source/WebCore/css/parser/CSSParserToken.cpp:429 >>> + appendCommentIfNeeded(nextTokenNeedsCommentTable, '-'); >> >> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this: >> >> appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken }); >> >> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list. > > Sure, that makes sense. Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >>>> Source/WebCore/css/parser/CSSParserToken.cpp:429 >>>> + appendCommentIfNeeded(nextTokenNeedsCommentTable, '-'); >>> >>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this: >>> >>> appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken }); >>> >>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list. >> >> Sure, that makes sense. > > Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time. I’m not sure why you’re saying this. We already do this for OptionSet; it works.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >>>>> Source/WebCore/css/parser/CSSParserToken.cpp:429 >>>>> + appendCommentIfNeeded(nextTokenNeedsCommentTable, '-'); >>>> >>>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this: >>>> >>>> appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken }); >>>> >>>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list. >>> >>> Sure, that makes sense. >> >> Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time. > > I’m not sure why you’re saying this. We already do this for OptionSet; it works. Ohh, you're saying buildNextTokenNeedsCommentTable should be converted into a class. Yeah, I think that would work.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >>>>>> Source/WebCore/css/parser/CSSParserToken.cpp:429 >>>>>> + appendCommentIfNeeded(nextTokenNeedsCommentTable, '-'); >>>>> >>>>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this: >>>>> >>>>> appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken }); >>>>> >>>>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list. >>>> >>>> Sure, that makes sense. >>> >>> Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time. >> >> I’m not sure why you’re saying this. We already do this for OptionSet; it works. > > Ohh, you're saying buildNextTokenNeedsCommentTable should be converted into a class. Yeah, I think that would work. Although, I think the downside is that it isn't guaranteed to be a constexpr value if you do it that way, AFAIK. It's possible it get generated that way in practice though.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review > Source/WebCore/css/parser/CSSParserToken.cpp:403 > +template<typename... CSSParserTokenTypes> > +constexpr std::array<bool, numberOfCSSParserTokenTypes> buildNextTokenNeedsCommentTable(CSSParserTokenTypes... nextTokenNeedsCommentTable) > +{ > + std::array<bool, numberOfCSSParserTokenTypes> table { false }; > + (table[nextTokenNeedsCommentTable] = ... = true); > + return table; > +} OK as is, but: Seems like this is *really* close to reinventing OptionSet. Seems a shame to build something so specific to this use when we would have something reusable. Also seems likely that using bits and shifting would be even more efficient than the arrays here, and the count here is comfortably under the 64 or 32 boundary.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >> Source/WebCore/css/parser/CSSParserToken.cpp:403 >> +} > > OK as is, but: Seems like this is *really* close to reinventing OptionSet. Seems a shame to build something so specific to this use when we would have something reusable. Also seems likely that using bits and shifting would be even more efficient than the arrays here, and the count here is comfortably under the 64 or 32 boundary. Per our offline discussion, it looks like converting this function into a struct with a constexpr constructor does happen to get optimized away (from reading the disassembly of this function). So I'm going to change this function into: ``` struct NextTokenNeedsCommentBuilder { constexpr NextTokenNeedsCommentBuilder(std::initializer_list<CSSParserTokenType> tokens) { for (auto token : tokens) buffer[token] = false; } std::array<bool, numberOfCSSParserTokenTypes> buffer { false }; }; ``` and have the lambda take a `const NextTokenNeedsCommentBuilder&` passed via the initializer_list as you recommended below.
Comment on attachment 409909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409909&action=review >>> Source/WebCore/css/parser/CSSParserToken.cpp:403 >>> +} >> >> OK as is, but: Seems like this is *really* close to reinventing OptionSet. Seems a shame to build something so specific to this use when we would have something reusable. Also seems likely that using bits and shifting would be even more efficient than the arrays here, and the count here is comfortably under the 64 or 32 boundary. > > Per our offline discussion, it looks like converting this function into a struct with a constexpr constructor does happen to get optimized away (from reading the disassembly of this function). So I'm going to change this function into: > > ``` > struct NextTokenNeedsCommentBuilder { > constexpr NextTokenNeedsCommentBuilder(std::initializer_list<CSSParserTokenType> tokens) > { > for (auto token : tokens) > buffer[token] = false; > } > > std::array<bool, numberOfCSSParserTokenTypes> buffer { false }; > }; > ``` > > and have the lambda take a `const NextTokenNeedsCommentBuilder&` passed via the initializer_list as you recommended below. Err, that loop should be `buffer[token] = true`. Turns out testing does help!
Created attachment 410048 [details] Patch for landing
Committed r267766: <https://trac.webkit.org/changeset/267766> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410048 [details].
<rdar://problem/69764946>