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

Support media queries binary logical operator syntax in platform expressions #267

Merged
merged 12 commits into from
Nov 17, 2021

Conversation

markle11m
Copy link
Contributor

This change implements support in platform expressions for binary logical operators (and, or) as defined by Media Queries.

We are extending current platform expressions as follows:

  • for logical-and, 'and' is a synonym for '&'
  • for logical-or, ',' is accepted, but has lower precedence than logical-and
    • currently using both '&' and '|' in the same expression is disallowed unless parens are used to identify evaluation precedence; that constraint is preserved, but '&', 'and', and ',' can be freely mixed in a platform expression without the need for parens

The existing platform expression parser:

  • does a left-to-right linear scan of the expression string
  • assumes that logical operators are individual chars ('!', '&', '|')
  • consumption/processing of binary logical operators is split between multiple methods (expr(), expr_binary()) and tightly couple with assumptions about valid syntax
  • does not create a binary expression tree
    • instead expr_binary() gathers consecutive instances of the same operation into the expression node (e.g., "A & B & C" yields <op_and, vector<A,B,C>>)
  • expr_binary() support backwards compatibility for chained vcpkg operators '&' and '|' (e.g., '&&&')
  • expr_binary() also detects the error case of mixing '&' and '|'
  • operator precedence is supported only explicitly (using parens, e.g., '(A & B) | C')

To support the media queries syntax, the following changes have been made:

  • moved all processing of logical-operators to a new function expr_operator()
    • removed the assumption that a logical operator is represented by an individual char (e.g., 'and')
    • added parsing support for 'and' and ','
  • added support for implicit operator precedence (the low-precedence or-operator ',')

Implementation details:

  • Added 16 unit tests for platform expressions, covering both existing and new syntax
  • Extended ExprKind to support low_precedence_or, empty and invalid operators to represent a more complete set of parse states after scanning for an expected operator
  • Added expr_operator() to extract the next operator from the input string. This is a destructive operation as it removes the chars parsed from the input text.
    • backwards compatible support for chained vcpkg operators '&' and '|' moved here from expr_binary()
    • expr_operator adds support for parsing non-single-char operators (e.g., "and")
      • note: 'and' is odd in that it's multiple chars and looks like an identifier, so it required special handling
  • Updated inaccurate error messages
  • Updated expr_binary() to template on ExprKind instead of char to support new operators and restrictions
    • chained vcpkg operator handling moved to expr_operator()
    • process ',' as 'platform_expression , platform_expression' to handle precedence order (as opposed to '|', which is
      implemented as 'plaform_expression_not | platform_expression_not'
    • consecutive instance gathering is supported for 'and' and ','

@markle11m markle11m marked this pull request as ready for review November 12, 2021 21:46
@@ -139,26 +142,114 @@ namespace vcpkg::PlatformExpression
// platform-expression =
// | platform-expression-not
// | platform-expression-and
// | platform-expression-or ;
// | platform-expression-or
// | platform-expression-low-precedence-or ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to write the real syntax; this isn't fully correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(probably in vcpkg)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will incorporate your suggested changes (in vcpkg) and update this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks mark!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after a few iterations, proposed EBNF update is posted, additional tests added and comments updated

src/vcpkg-test/platform-expression.cpp Show resolved Hide resolved
src/vcpkg/platform-expression.cpp Outdated Show resolved Hide resolved
src/vcpkg/platform-expression.cpp Outdated Show resolved Hide resolved

m_expr = parse_expr("notANY windows");
CHECK_FALSE(m_expr);
m_expr = parse_expr("not! windows");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one invalid? Shouldn't this parse as !!windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. At first glance, one would expect not! windows to be the same as !!windows and evaluate to windows. But the code currently fails to part both of those expressions. It also doesn't appear to be a valid parse according to the EBNF.
So I think you're right and this should be valid, but we'll need to update the grammar when we do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants