[DAPHNE-#560] Parsing unary minus operator #607
Conversation
|
I could fix the script argument test by some more DSL grammar changes. But the matrix literals remain an issue. Similar to #582, this functionality relies on constants to generate data, which is something we can not provide in the parsing stage (current fix would need a round of constant folding to do the multiply with -1). |
corepointer
force-pushed
the
560-parsing-unary-minus
branch
from
de078b6
to
36b1695
Compare
This change decouples the '-' from INT_LITERAL and FLOAT_LITERAL and handles a unary minus separately. The test case covers a collection of cases where a unary minus is used and also documents where it still does not work. This change also makes the command line argument an expression, so simple mathematical expressions like 2+2 will be evaluated. Resolves daphne-eu#560
pdamme
force-pushed
the
560-parsing-unary-minus
branch
3 times, most recently
from
13c8124
to
6562235
Compare
- Undid unrelated changes in run-docker-example.sh.
- DaphneDSL parser
- DaphneDSL grammar
- Moved the new unary -/+ operators down in the expression precedence (should not have an effect, since no conflict with the expression types above, but more intuitive since close to the binary operators).
- Renamed unaryPlusMinusExpr to minusExpr (for consistency with the other names) and rhs to arg in minusExpr (since there is only one argument).
- Corrected INT_LITERAL lexer rule: removed duplicate type suffix.
- Removed minus sign also from FLOAT_LITERAL lexer rule.
- Otherwise expressions like `3.0-1.0` (as opposed to `3.0 - 1.0`) still lead to the parser confusion about binary vs. unary minus.
- DaphneDSL visitors
- Instead of a multiplication by -1, the unary minus operator results in a additive inverse operation (EwMinusOp).
- The kernel support for that required only a few lines.
- Besides being simpler, this change was necessary as:
- The previous code needed to known the type of the arg of unary minus, but:
- Fristly, we may not know the argument's type at parse-time.
- Secondly, it's not good to hardcode the supported values types, since the set of supported value types may change.
- Thirdly, unary minus did not work for matrices, now it does.
- An attempt to multiply a -1 of type si8 (the least general value type w.r.t. type promotion) to an arg of any type failed to due to a bug in constant folding (daphne-eu#772).
- Generating exceptions via ErrorHandler.
- Undid feature of arbitrary expressions as script args, since this can have unwanted effects (code injection etc.).
- Instead, only a leading minus is allowed in args and is treated specially by creating a EwMinusOp around the parsed literal.
- Added test cases for that, but discovered a related bug beyond the scope of this PR (daphne-eu#773); thus the test cases are commented out for now.
- Moved visitor function within the files for consistency with the order of the expression types in the grammar.
- Added a more detailed explanation to the new special case for parsing integer literals.
- Runtime
- Added unary minus (additive inverse) operation as a new op code for the ewUnarySca/ewUnaryMat kernels.
- Tests
- Removed the new test cases in test/api/cli/parser/, since this is a very unspecific location.
- Moved decisive parts of these tests to more specific locations, like test/api/cli/operations/ and test/api/cli/literals/.
- Extended the test cases.
- Updated DaphneDSL language reference.
- No changes to the description of literals: leading minus is still presented as a part of the literal here, since this is more intuitive from a user's point of view.
- Mentioning unary -/+ in the operators table.
- And some more minor things.
pdamme
force-pushed
the
560-parsing-unary-minus
branch
from
6562235
to
8775138
Compare
There was a problem hiding this comment.
Thanks for this fix, @corepointer! It basically looked good, but required some additional work to make it more robust.
For instance, some fixes in the grammar, support for unary minus before matrices (not only scalars), implementation of the unary minus via EwMinusOp instead of multiplication by -1 to avoid some type problems, restriction of script arguments to literals optionally preceded by a minus (as opposed to arbitrary expressions, as those could have unwanted side effects), more test cases, updates to the DaphneDSL docs. For more details, see the description of the commit I added.
As that was quite some work compared to the initial state of the PR, I take the liberty to add myself as a co-author of the resulting commit on main. This PR is ready to be merged now.
|
The failed CI run is most likely due to #729, so I will merge it anyway. |
This change decouples the '-' from INT_LITERAL and FLOAT_LITERAL and handles a unary minus separately.
Testcases... I know... 🙄