Adds macro evaluator implementation #924
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #924 +/- ##
==================================================
Coverage ? 69.63%
Complexity ? 6628
==================================================
Files ? 190
Lines ? 26195
Branches ? 4724
==================================================
Hits ? 18240
Misses ? 6535
Partials ? 1420 ☔ View full report in Codecov by Sentry. |
| // SPDX-License-Identifier: Apache-2.0 | ||
| package com.amazon.ion.impl.macro | ||
|
|
||
| data class Environment private constructor( |
| assertEquals(StringValue(value = "c"), evaluator.expandNext()) | ||
| assertEquals(null, evaluator.expandNext()) | ||
| evaluator.stepOut() |
There was a problem hiding this comment.
Is early step out supported? If so, let's add a test for that.
There was a problem hiding this comment.
It is supported. I'll add that.
| // Given: | ||
| // (macro foo_struct (x*) {foo: x}) | ||
| // When: | ||
| // (:foo_struct (: bar baz)) |
There was a problem hiding this comment.
Not relevant to this PR, but the more I look at this syntax for an argument group, the less I like it... we've been trained that whitespace after a colon doesn't matter, except here it does.
There was a problem hiding this comment.
That's why I tried to propose [:
| // Yes, the field name should be here only once. The Ion reader that wraps the evaluator | ||
| // is responsible for carrying the field name over to any values that follow. |
There was a problem hiding this comment.
That's the API that I came up with. 🤷♂️
To make it easier for me to ensure correctness, I started by having very segregated concerns. The MacroEvaluator class is only concerned with combining Expressions. The MacroEvaluatorAsIonReader class is responsible for taking the MacroEvaluator output and making it fit the IonReader expectations. It doesn't have to be this way, and if we merge the MacroEvaluatorAsIonReader class with MacroEvaluator, then this will no longer be relevant.
| // Yes, the field name should be here. The Ion reader that wraps the evaluator | ||
| // is responsible for discarding the field name if no values follow. |
There was a problem hiding this comment.
See above—same reason.
| macroEvaluator.expansionStack.peek().expansionKind = ExpansionKind.Values | ||
| val minDepth = macroEvaluator.expansionStack.size() | ||
| // ...But capture the output and turn it into a String | ||
| val sb = StringBuilder() |
There was a problem hiding this comment.
Looking forward to performance tuning, maybe we could have a reusable StringBuilder that is used for all make_string expansions.
There was a problem hiding this comment.
We could run into some problems with macros that are composed of other macros when the is a nested make_string in the arguments for a make_string, but we could certainly see about a StringBuilder pool.
There was a problem hiding this comment.
Or one StringBuilder for each nesting depth?
There was a problem hiding this comment.
Yeah, that might work.
| Placeholder -> TODO("unreachable") | ||
| is MacroInvocation -> pushTdlMacroExpansion(currentExpr) | ||
| is EExpression -> pushEExpressionExpansion(currentExpr) | ||
| is VariableRef -> pushVariableExpansion(currentExpr) |
There was a problem hiding this comment.
When/where is the variable substituted?
There was a problem hiding this comment.
In pushVariableExpansion, it gets the argument expressions from the environment and pushes them into the expansion stack.
Issue #, if available:
#730, #731
Description of changes:
An initial macro evaluator implementation.
There are a few things that should be improved, but I haven't done them yet.
Expanderinterface and implementations out ofMacroEvaluatorbefore adding more system macros so thatMacroEvaluator.ktdoesn't get ridiculously large.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.