Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow parser to support generic data types #48718

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NickGerleman
Copy link
Contributor

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.

struct CSSNumber {
  float value{};
};

template <>
struct CSSDataTypeParser<CSSNumber> {
  static constexpr std::optional<CSSNumber> consumePreservedToken(
      const CSSPreservedToken& token) {
    if (token.type() == CSSTokenType::Number) {
      return CSSNumber{token.numericValue()};
    }

    return {};
  }
};

static_assert(CSSDataType<CSSNumber>);

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 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

NickGerleman and others added 3 commits January 15, 2025 17:36
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:
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.

```
struct CSSNumber {
  float value{};
};

template <>
struct CSSDataTypeParser<CSSNumber> {
  static constexpr std::optional<CSSNumber> consumePreservedToken(
      const CSSPreservedToken& token) {
    if (token.type() == CSSTokenType::Number) {
      return CSSNumber{token.numericValue()};
    }

    return {};
  }
};

static_assert(CSSDataType<CSSNumber>);
```

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 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
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Jan 16, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D68245734

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants