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

Use add/set_macros/symbols in ManagedWriter #987

Conversation

jobarr-amzn
Copy link
Contributor

Description of changes:
Rather than rendering an encoding context directive the managed writer now uses set_symbols, set_macros, add_symbols, or add_macros as appropriate. Symbols and macros are not set until the user defines a symbol or macro- until that point system symbols and macros are still more succinctly expressible.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

(symbol_table ["foo","bar","baz"])
(macro_table
(:$ion::set_symbols
(:: "foo" "bar" "baz"))
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could have it write it like this instead, but we can leave that for another day.

(:set_symbols "foo" "bar" "baz")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into that briefly- in some cases it is being written that way, so it looks like there's some more thought needed on the tuning or something else I missed.

jobarr-amzn and others added 2 commits October 31, 2024 12:01
@@ -155,7 +155,7 @@ internal class IonManagedWriter_1_1_Test {
fun `write an encoding directive with a non-empty macro table`() {
val expected = """
$ion_1_1
$ion_encoding::((macro_table (macro null () "foo")))
(:$ion::set_macros (:: (macro null () "foo")))
Copy link
Contributor

Choose a reason for hiding this comment

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

In these cases, the $ion:: prefix is not required, right, since the IVM installs the system module? This can be out of scope for this PR, but we might as well refine this at some point to be as succinct as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's the raw writer that is determining this behavior. I'd also like to make this more succinct.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the managed writer that needs to determine the behavior because the raw writer should not know whether something is in the macro table or not.

The stepInEExp(SystemMacro) method in the raw writer must always write a system e-expression using the specific text or binary syntax for a system e-exp (which is the current behavior).

To get rid fo the unnecessary $ion prefix, the managed writer need to check whether the desired system macro is in the encoding module. If it is in the encoding module, then the managed writer can use stepInEExp(CharSequence) or stepInEExp(Int, Boolean, Macro), similar to the logic in startMacro (see below). If the system macro is not in the encoding module, then the managed writer needs to call stepInEExp(SystemMacro) (which results in the qualified invocation in text or the EF opcode in binary).

private fun startMacro(name: String?, address: Int, definition: Macro) {
val useNames = options.eExpressionIdentifierStrategy == ManagedWriterOptions_1_1.EExpressionIdentifierStrategy.BY_NAME
if (useNames && name != null) {
userData.stepInEExp(name)
} else {
val includeLengthPrefix = options.writeLengthPrefix(ContainerType.EEXP, depth + 1)
userData.stepInEExp(address, includeLengthPrefix, definition)
}
}

@jobarr-amzn jobarr-amzn merged commit d434d2a into amazon-ion:ion-11-encoding Oct 31, 2024
15 of 34 checks passed
@jobarr-amzn jobarr-amzn deleted the ion-11-managed-system-macros branch October 31, 2024 22:13
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