Implements flatten, make_list, meta
#874
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #874 +/- ##
==========================================
+ Coverage 77.61% 77.68% +0.06%
==========================================
Files 136 136
Lines 34141 34188 +47
Branches 34141 34188 +47
==========================================
+ Hits 26498 26558 +60
+ Misses 5666 5648 -18
- Partials 1977 1982 +5 ☔ View full report in Codecov by Sentry. |
| @@ -438,7 +438,7 @@ mod benchmark { | |||
| let mut context = EncodingContext::for_ion_version(IonVersion::v1_1); | |||
| context | |||
| .macro_table_mut() | |||
| .add_macro(compiled_macro.clone()) | |||
| .add_template_macro(compiled_macro.clone()) | |||
There was a problem hiding this comment.
🪧 This method (which is specific to template macros) was renamed to accommodate adding a more general purpose add_macro method alongside it.
| MacroKind::MakeSExp => MacroExpansionKind::MakeSExp(MakeSExpExpansion::new(arguments)), | ||
| MacroKind::Annotate => MacroExpansionKind::Annotate(AnnotateExpansion::new(arguments)), | ||
| MacroKind::Flatten => MacroExpansionKind::Flatten(FlattenExpansion::new( | ||
| self.context, | ||
| environment, | ||
| arguments, | ||
| )), |
There was a problem hiding this comment.
🪧 Now that make_sexp is a trivial template that uses flatten, we no longer need a special MacroKind and custom impl for it. While this PR adds 3 new system macros, MacroKind's number of variants is unchanged.
| #[derive(Copy, Clone, Debug)] | ||
| #[derive(Debug)] |
There was a problem hiding this comment.
🪧 flatten is the first system macro to be implemented which requires a macro evaluator to persist across calls to evaluator.next(). As a result, the MacroExpansion family of types can no longer implement Copy, as each one might own a bump-allocated evaluator of its own. The Copy impl was only being used directly in one speculatively defined function that had no callers. I'll point it out later in the diff.
|
|
||
| // Calculate the next step in this macro expansion without advancing the expansion. | ||
| pub fn peek_next_step(&self) -> IonResult<MacroExpansionStep<'top, D>> { | ||
| let mut expansion_copy = *self; |
There was a problem hiding this comment.
🪧 The aforementioned use of Copy, which would have enabled peek()ing at the next step of a macro. This is still possible without Copy, but will take a bit more code.
| match self.state { | ||
| match mem::take(&mut self.state) { |
There was a problem hiding this comment.
🪧 An indirect use of Copy: because self.state isn't Copy any more, we need to set its value to the default (Empty) so we can update its state using the (owned) contents.
| pub const SYSTEM_MACRO_KINDS: &'static [MacroKind] = &[ | ||
| MacroKind::None, | ||
| MacroKind::ExprGroup, | ||
| MacroKind::MakeString, | ||
| MacroKind::MakeSExp, | ||
| MacroKind::Annotate, | ||
| ]; |
There was a problem hiding this comment.
🪧 This was a holdover from before we had a proper system macro table. It's no longer in use.
| @@ -511,18 +528,21 @@ impl MacroTable { | |||
| Ok(id) | |||
| } | |||
|
|
|||
| pub(crate) fn append_macro(&mut self, macro_ref: &Arc<Macro>) -> IonResult<()> { | |||
There was a problem hiding this comment.
🪧 This method was extracted from the append_all_macros_from function below. It is now also be used in compile_system_macros().
| pub fn add_macro(&mut self, template: TemplateMacro) -> IonResult<usize> { | ||
| pub fn add_template_macro(&mut self, template: TemplateMacro) -> IonResult<usize> { |
There was a problem hiding this comment.
🪧 As mentioned prior, this was renamed to accommodate the introduction of a more general append_macro method (see below).
| /// The SExp was produced by a call to `make_sexp`. | ||
| Constructed(Environment<'top, D>, MacroExprArgsIterator<'top, D>), |
There was a problem hiding this comment.
Defining make_sexp in terms of flatten allowed me to delete lots of special-case code for make_sexp. 🎉
| self.context | ||
| .macro_table | ||
| .add_template_macro(new_macro) | ||
| .unwrap(); |
Changes:
flattenmacromake_listas a template that leveragesflattenmake_sexpas a template that leveragesflattenmake_sexpmetaas a template that leveragesnoneBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.