Allow parser to support generic data types #48718
Closed
+1,207
−2,953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request was exported from Phabricator. Differential Revision: D68245734 |
Summary: `transparent` is specced as a special `<named-color>` outside the table the others were derived from. Let's add it, since it is supported today by `normalizeColor`. https://www.w3.org/TR/css-color-4/#named-colors Changelog: [Internal] Differential Revision: D68246770
Summary: tsia Changelog: [Internal] Differential Revision: D68246769
Summary: Pull Request resolved: facebook#48718 This diff looks a bit scary, but it's mostly just structural changes of existing code and some deletion, on a well tested path 😅. The current parser implementation tries to special case "basic" data types. When I was looking at how to add in support for more complex values, such as lists, function notations, and more compounded types, this distinction ends up not making much sense. Instead of treating some types as basic, this diff instead moves to a model where a user can declare any structure as a `CSSDataType`, so long as they also supply a parser, which may be visited when iterating through CSS syntax blocks (preserved tokens, function blocks, or simple blocks, which probably won't be used). The user then specifies a list of supported CSS data types to parse, which invokes said parser, calling any defined methods for specific syntax. E.g. ```cpp struct CSSNumber { float value{}; }; template <> struct CSSDataTypeParser<CSSNumber> { static constexpr auto consumePreservedToken(const CSSPreservedToken& token) -> std::optional<CSSNumber> { if (token.type() == CSSTokenType::Number) { return CSSNumber{token.numericValue()}; } return {}; } // Could also accept function block here as well (e.g. for future math // expressions) }; static_assert(CSSDataType<CSSNumber>); ``` ```cpp // Can be one of std::monostate (variant null-type), CSSWideKeyword, // CSSNumber, CSSLength, or CSSPercentage. In this case, a CSSLength. auto value = parseCSSProperty<CSSNumber, CSSLength, CSSPercentage>("5px"); ``` This breaks a whole lot of assumptions I made a year ago, especially around `CSSValueVariant` which must now be able to handle arbitrary values. For now, for the sake of simplicity, I threw this out, and migrated parser code to use plain-old `std::variant`, which has a downside of being a bit less optimized in terms of storage. I also ended up completely throwing out `CSSDeclaredStyle`, since it would majorly need to change, and we're not going to be migrating style storage quite yet. This change also broke the `CSSProperties.h` property definitions and parsing shorthand a bit, which we will need for value processing later. I also opted to delete this for now (a big centralized list is the wrong structure anyways), but will likely copy bits from its source history later. Another particular hairy bit, that likely won't bite us in practice, is that some strings may be parseable under different data types. This just adds caller requirement to order the types correctly, instead of precedence being implemented as part of the parser. Changelog: [Internal] Reviewed By: lenaic Differential Revision: D68245734
|
This pull request was exported from Phabricator. Differential Revision: D68245734 |
7f39ec7 to
725b515
Compare
|
This pull request has been merged in 69f761f. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This diff looks a bit scary, but it's mostly just structural changes of existing code and some deletion, on a well tested path 😅.
The current parser implementation tries to special case "basic" data types. When I was looking at how to add in support for more complex values, such as lists, function notations, and more compounded types, this distinction ends up not making much sense.
Instead of treating some types as basic, this diff instead moves to a model where a user can declare any structure as a
CSSDataType, so long as they also supply a parser, which may be visited when iterating through CSS syntax blocks. The user then specifies a list of supported CSS data types to parse, which invokes said parser. E.g.This breaks a whole lot of assumptions I made a year ago, especially around
CSSValueVariantwhich must now be able to handle arbitrary values. For now, for the sake of simplicity, I threw this out, and migrated parser code to use plain-oldstd::variant, which has a downside of being a bit less optimized in terms of storage. I also ended up completely throwing outCSSDeclaredStyle, since it would majorly need to change, and we're not going to be migrating style storage quite yet. This change also broke theCSSProperties.hproperty definitions a bit, which we will need for value processing later. I also opted to delete this for now, but will likely copy bits from it for source history later.Another particular hairy bit, that likely won't bite us in practice, is that some strings may be parseable under different data types. This just adds caller requirement to order the types correctly, instead of precedence being implemented as part of the parser.
Changelog: [Internal]
Differential Revision: D68245734