Introduce a BlockLocalVariableNode

This is a tradeoff that I think is worth it. Right now we have a location list that tracks the location of each of the block locals. Instead, I'd like to make that a node list that has a proper node in each spot in the list. In doing so, we eliminate the need to have a location list at all, making it simpler on all of the various consumers as we have one fewer field type. There should be minimal memory implications here since this syntax is exceedingly rare.
ruby · Sep 5, 2023 · 04d329d · 04d329d
1 parent 38c20cf
commit 04d329d
Show file tree

Hide file tree

Showing 29 changed files with 78 additions and 176 deletions.
diff --git a/bin/memsize b/bin/memsize
@@ -16,7 +16,7 @@ results =
           8
         when "location", "location?"
           16
-        when "node[]", "string", "token", "token?", "location[]", "constant[]"
+        when "node[]", "string", "token", "token?", "constant[]"
           24
         when "flags"
           0

diff --git a/config.yml b/config.yml
@@ -520,6 +520,15 @@ nodes:
 
           bar(&args)
           ^^^^^^^^^^
+  - name: BlockLocalVariableNode
+    fields:
+      - name: name
+        type: constant
+    comment: |
+      Represents a block local variable.
+
+          a { |; b| }
+                 ^
   - name: BlockNode
     fields:
       - name: locals
@@ -556,7 +565,7 @@ nodes:
         type: node?
         kind: ParametersNode
       - name: locals
-        type: location[]
+        type: node[]
       - name: opening_loc
         type: location?
       - name: closing_loc

diff --git a/docs/configuration.md b/docs/configuration.md
@@ -51,7 +51,6 @@ The available values for `type` are:
 * `constant[]` - A child node that is an array of constants. This is a `yp_constant_id_list_t` in C.
 * `location` - A child node that is a location. This is a `yp_location_t` in C.
 * `location?` - A child node that is a location that is optionally present. This is a `yp_location_t` in C, but if the value is not present then the `start` and `end` fields will be `NULL`.
-* `location[]` - A child node that is an array of locations. This is a `yp_location_list_t` in C.
 * `uint32` - A child node that is a 32-bit unsigned integer. This is a `uint32_t` in C.
 
 If the type is `node` or `node?` then the value also accepts an optional `kind` key (a string). This key is expected to match to the name of another node type within `config.yml`. This changes a couple of places where code is templated out to use the more specific struct name instead of the generic `yp_node_t`. For example, with `kind: StatementsNode` the `yp_node_t *` in C becomes a `yp_statements_node_t *`.
diff --git a/docs/serialization.md b/docs/serialization.md
@@ -93,7 +93,6 @@ Depending on the type of child node, it could take a couple of different forms,
 * `constant[]` - A child node that is an array of constants. This is structured as a variable-length integer length, followed by the child constants themselves.
 * `location` - A child node that is a location. This is structured as a variable-length integer start followed by a variable-length integer length.
 * `location?` - A child node that is a location that is optionally present. If the location is not present, then a single `0` byte will be written in its place. If it is present, then it will be structured just like the `location` child node.
-* `location[]` - A child node that is an array of locations. This is structured as a `4` byte length, followed by the locations themselves.
 * `uint32` - A child node that is a 32-bit unsigned integer. This is structured as a variable-length integer.
 
 After the syntax tree, the content pool is serialized.

diff --git a/include/yarp/node.h b/include/yarp/node.h
@@ -4,9 +4,6 @@
 #include "yarp/defines.h"
 #include "yarp/parser.h"
 
-// Append a token to the given list.
-void yp_location_list_append(yp_location_list_t *list, const yp_token_t *token);
-
 // Append a new node onto the end of the node list.
 void yp_node_list_append(yp_node_list_t *list, yp_node_t *node);
 
@@ -31,7 +28,6 @@ YP_EXPORTED_FUNCTION void yp_node_memsize(yp_node_t *node, yp_memsize_t *memsize
 YP_EXPORTED_FUNCTION const char * yp_node_type_to_str(yp_node_type_t node_type);
 
 #define YP_EMPTY_NODE_LIST ((yp_node_list_t) { .nodes = NULL, .size = 0, .capacity = 0 })
-#define YP_EMPTY_LOCATION_LIST ((yp_location_list_t) { .locations = NULL, .size = 0, .capacity = 0 })
 
 #endif // YARP_NODE_H
 

diff --git a/rust/yarp/build.rs b/rust/yarp/build.rs
@@ -34,9 +34,6 @@ enum NodeFieldType {
     #[serde(rename = "location?")]
     OptionalLocation,
 
-    #[serde(rename = "location[]")]
-    LocationList,
-
     #[serde(rename = "uint32")]
     UInt32,
 
@@ -254,12 +251,6 @@ fn write_node(file: &mut File, node: &Node) -> Result<(), Box<dyn std::error::Er
                 writeln!(file, "        }}")?;
                 writeln!(file, "    }}")?;
             },
-            NodeFieldType::LocationList => {
-                writeln!(file, "    pub fn {}(&self) -> LocationList<'pr> {{", field.name)?;
-                writeln!(file, "        let pointer: *mut yp_location_list_t = unsafe {{ &mut (*self.pointer).{} }};", field.name)?;
-                writeln!(file, "        LocationList {{ pointer: unsafe {{ NonNull::new_unchecked(pointer) }}, marker: PhantomData }}")?;
-                writeln!(file, "    }}")?;
-            },
             NodeFieldType::UInt32 => {
                 writeln!(file, "    pub fn {}(&self) -> u32 {{", field.name)?;
                 writeln!(file, "        unsafe {{ (*self.pointer).{} }}", field.name)?;
@@ -454,51 +445,6 @@ impl std::fmt::Debug for Location<'_> {{
     }}
 }}
 
-/// An iterator over the ranges in a list.
-pub struct LocationListIter<'pr> {{
-    pointer: NonNull<yp_location_list_t>,
-    index: usize,
-    marker: PhantomData<&'pr mut yp_location_list_t>
-}}
-
-impl<'pr> Iterator for LocationListIter<'pr> {{
-    type Item = Location<'pr>;
-
-    fn next(&mut self) -> Option<Self::Item> {{
-        if self.index >= unsafe {{ self.pointer.as_ref().size }} {{
-            None
-        }} else {{
-            let pointer: *mut yp_location_t = unsafe {{ self.pointer.as_ref().locations.add(self.index) }};
-            self.index += 1;
-            Some(Location {{ pointer: unsafe {{ NonNull::new_unchecked(pointer) }}, marker: PhantomData }})
-        }}
-    }}
-}}
-
-/// A list of ranges in the source file.
-pub struct LocationList<'pr> {{
-    pointer: NonNull<yp_location_list_t>,
-    marker: PhantomData<&'pr mut yp_location_list_t>
-}}
-
-impl<'pr> LocationList<'pr> {{
-    /// Returns an iterator over the locations.
-    #[must_use]
-    pub fn iter(&self) -> LocationListIter<'pr> {{
-        LocationListIter {{
-            pointer: self.pointer,
-            index: 0,
-            marker: PhantomData
-        }}
-    }}
-}}
-
-impl std::fmt::Debug for LocationList<'_> {{
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {{
-        write!(f, "{{:?}}", self.iter().collect::<Vec<_>>())
-    }}
-}}
-
 /// An iterator over the nodes in a list.
 pub struct NodeListIter<'pr> {{
     parser: NonNull<yp_parser_t>,

diff --git a/rust/yarp/src/lib.rs b/rust/yarp/src/lib.rs
@@ -262,29 +262,6 @@ mod tests {
         assert_eq!(slice, "222");
     }
 
-    #[test]
-    fn location_list_test() {
-        use super::{
-            Visit,
-            visit_block_parameters_node, BlockParametersNode
-        };
-
-        struct BlockLocalsVisitor {}
-
-        impl Visit<'_> for BlockLocalsVisitor {
-            fn visit_block_parameters_node(&mut self, node: &BlockParametersNode<'_>) {
-                println!("{:?}", node.locals());
-                visit_block_parameters_node(self, node);
-            }
-        }
-
-        let source = "-> (; foo, bar) {}";
-        let result = parse(source.as_ref());
-
-        let mut visitor = BlockLocalsVisitor {};
-        visitor.visit(&result.node());
-    }
-
     #[test]
     fn visitor_test() {
         use super::{

diff --git a/src/yarp.c b/src/yarp.c
@@ -1175,7 +1175,7 @@ yp_block_parameters_node_create(yp_parser_t *parser, yp_parameters_node_t *param
         .parameters = parameters,
         .opening_loc = YP_OPTIONAL_LOCATION_TOKEN_VALUE(opening),
         .closing_loc = { .start = NULL, .end = NULL },
-        .locals = YP_EMPTY_LOCATION_LIST
+        .locals = YP_EMPTY_NODE_LIST
     };
 
     return node;
@@ -1190,14 +1190,30 @@ yp_block_parameters_node_closing_set(yp_block_parameters_node_t *node, const yp_
     node->closing_loc = YP_LOCATION_TOKEN_VALUE(closing);
 }
 
+// Allocate and initialize a new BlockLocalVariableNode node.
+static yp_block_local_variable_node_t *
+yp_block_local_variable_node_create(yp_parser_t *parser, const yp_token_t *name) {
+    assert(name->type == YP_TOKEN_IDENTIFIER || name->type == YP_TOKEN_MISSING);
+    yp_block_local_variable_node_t *node = YP_ALLOC_NODE(parser, yp_block_local_variable_node_t);
+
+    *node = (yp_block_local_variable_node_t) {
+        {
+            .type = YP_NODE_BLOCK_LOCAL_VARIABLE_NODE,
+            .location = YP_LOCATION_TOKEN_VALUE(name),
+        },
+        .name = yp_parser_constant_id_token(parser, name)
+    };
+
+    return node;
+}
+
 // Append a new block-local variable to a BlockParametersNode node.
 static void
-yp_block_parameters_node_append_local(yp_block_parameters_node_t *node, const yp_token_t *local) {
-    assert(local->type == YP_TOKEN_IDENTIFIER || local->type == YP_TOKEN_MISSING);
+yp_block_parameters_node_append_local(yp_block_parameters_node_t *node, const yp_block_local_variable_node_t *local) {
+    yp_node_list_append(&node->locals, (yp_node_t *) local);
 
-    yp_location_list_append(&node->locals, local);
-    if (node->base.location.start == NULL) node->base.location.start = local->start;
-    node->base.location.end = local->end;
+    if (node->base.location.start == NULL) node->base.location.start = local->base.location.start;
+    node->base.location.end = local->base.location.end;
 }
 
 // Allocate and initialize a new BreakNode node.
@@ -9283,7 +9299,9 @@ parse_block_parameters(
         do {
             expect(parser, YP_TOKEN_IDENTIFIER, "Expected a local variable name.");
             yp_parser_local_add_token(parser, &parser->previous);
-            yp_block_parameters_node_append_local(block_parameters, &parser->previous);
+
+            yp_block_local_variable_node_t *local = yp_block_local_variable_node_create(parser, &parser->previous);
+            yp_block_parameters_node_append_local(block_parameters, local);
         } while (accept(parser, YP_TOKEN_COMMA));
     }
 

diff --git a/templates/ext/yarp/api_node.c.erb b/templates/ext/yarp/api_node.c.erb
@@ -152,12 +152,6 @@ yp_ast_new(yp_parser_t *parser, yp_node_t *node, rb_encoding *encoding) {
                     }
                     <%- when YARP::StringField -%>
                     argv[<%= index %>] = yp_string_new(&cast-><%= field.name %>, encoding);
-                    <%- when YARP::LocationListField -%>
-                    argv[<%= index %>] = rb_ary_new_capa(cast-><%= field.name %>.size);
-                    for (size_t index = 0; index < cast-><%= field.name %>.size; index++) {
-                        yp_location_t location = cast-><%= field.name %>.locations[index];
-                        rb_ary_push(argv[<%= index %>], yp_location_new(parser, location.start, location.end, source));
-                    }
                     <%- when YARP::ConstantField -%>
                     argv[<%= index %>] = rb_id2sym(constants[cast-><%= field.name %> - 1]);
                     <%- when YARP::ConstantListField -%>

diff --git a/templates/include/yarp/ast.h.erb b/templates/include/yarp/ast.h.erb
@@ -32,12 +32,6 @@ typedef struct {
     const uint8_t *end;
 } yp_location_t;
 
-typedef struct {
-    yp_location_t *locations;
-    size_t size;
-    size_t capacity;
-} yp_location_list_t;
-
 struct yp_node;
 
 typedef struct yp_node_list {
@@ -95,7 +89,6 @@ typedef struct yp_<%= node.human %> {
     <%= case field
     when YARP::NodeField, YARP::OptionalNodeField then "struct #{field.c_type} *#{field.name}"
     when YARP::NodeListField then "struct yp_node_list #{field.name}"
-    when YARP::LocationListField then "yp_location_list_t #{field.name}"
     when YARP::ConstantField then "yp_constant_id_t #{field.name}"
     when YARP::ConstantListField then "yp_constant_id_list_t #{field.name}"
     when YARP::StringField then "yp_string_t #{field.name}"

diff --git a/templates/java/org/yarp/Loader.java.erb b/templates/java/org/yarp/Loader.java.erb
@@ -263,7 +263,6 @@ public class Loader {
               when YARP::OptionalNodeField then "#{field.java_cast}loadOptionalNode()"
               when YARP::StringField then "loadString()"
               when YARP::NodeListField then "loadNodes()"
-              when YARP::LocationListField then "loadLocations()"
               when YARP::ConstantField then "loadConstant()"
               when YARP::ConstantListField then "loadConstants()"
               when YARP::LocationField then "loadLocation()"

diff --git a/templates/lib/yarp/node.rb.erb b/templates/lib/yarp/node.rb.erb
@@ -100,7 +100,7 @@ module YARP
       <%- case field -%>
       <%- when YARP::NodeListField -%>
       inspector << "<%= pointer %><%= field.name %>: #{inspector.list("#{inspector.prefix}<%= preadd %>", <%= field.name %>)}"
-      <%- when YARP::LocationListField, YARP::ConstantListField -%>
+      <%- when YARP::ConstantListField -%>
       inspector << "<%= pointer %><%= field.name %>: #{<%= field.name %>.inspect}\n"
       <%- when YARP::NodeField -%>
       inspector << "<%= pointer %><%= field.name %>:\n"

diff --git a/templates/lib/yarp/serialize.rb.erb b/templates/lib/yarp/serialize.rb.erb
@@ -185,7 +185,6 @@ module YARP
             when YARP::OptionalNodeField then "load_optional_node"
             when YARP::StringField then "load_string"
             when YARP::NodeListField then "Array.new(load_varint) { load_node }"
-            when YARP::LocationListField then "Array.new(load_varint) { load_location }"
             when YARP::ConstantField then "load_constant"
             when YARP::ConstantListField then "Array.new(load_varint) { load_constant }"
             when YARP::LocationField then "load_location"

diff --git a/templates/src/node.c.erb b/templates/src/node.c.erb
@@ -8,30 +8,6 @@ void yp_node_clear(yp_node_t *node) {
     node->location = location;
 }
 
-// Calculate the size of the token list in bytes.
-static size_t
-yp_location_list_memsize(yp_location_list_t *list) {
-    return sizeof(yp_location_list_t) + (list->capacity * sizeof(yp_location_t));
-}
-
-// Append a token to the given list.
-void
-yp_location_list_append(yp_location_list_t *list, const yp_token_t *token) {
-    if (list->size == list->capacity) {
-        list->capacity = list->capacity == 0 ? 2 : list->capacity * 2;
-        list->locations = (yp_location_t *) realloc(list->locations, sizeof(yp_location_t) * list->capacity);
-    }
-    list->locations[list->size++] = (yp_location_t) { .start = token->start, .end = token->end };
-}
-
-// Free the memory associated with the token list.
-static void
-yp_location_list_free(yp_location_list_t *list) {
-    if (list->locations != NULL) {
-        free(list->locations);
-    }
-}
-
 static void
 yp_node_memsize_node(yp_node_t *node, yp_memsize_t *memsize);
 
@@ -95,8 +71,6 @@ yp_node_destroy(yp_parser_t *parser, yp_node_t *node) {
             yp_string_free(&cast-><%= field.name %>);
             <%- when YARP::NodeListField -%>
             yp_node_list_free(parser, &cast-><%= field.name %>);
-            <%- when YARP::LocationListField -%>
-            yp_location_list_free(&cast-><%= field.name %>);
             <%- when YARP::ConstantListField -%>
             yp_constant_id_list_free(&cast-><%= field.name %>);
             <%- else -%>
@@ -141,8 +115,6 @@ yp_node_memsize_node(yp_node_t *node, yp_memsize_t *memsize) {
             memsize->memsize += yp_string_memsize(&cast-><%= field.name %>);
             <%- when YARP::NodeListField -%>
             yp_node_list_memsize(&cast-><%= field.name %>, memsize);
-            <%- when YARP::LocationListField -%>
-            memsize->memsize += yp_location_list_memsize(&cast-><%= field.name %>);
             <%- when YARP::ConstantListField -%>
             memsize->memsize += yp_constant_id_list_memsize(&cast-><%= field.name %>);
             <%- else -%>

diff --git a/templates/src/prettyprint.c.erb b/templates/src/prettyprint.c.erb
@@ -45,13 +45,6 @@ prettyprint_node(yp_buffer_t *buffer, yp_parser_t *parser, yp_node_t *node) {
                 prettyprint_node(buffer, parser, (yp_node_t *) ((yp_<%= node.human %>_t *) node)-><%= field.name %>.nodes[index]);
             }
             yp_buffer_append_str(buffer, "]", 1);
-            <%- when YARP::LocationListField -%>
-            yp_buffer_append_str(buffer, "[", 1);
-            for (uint32_t index = 0; index < ((yp_<%= node.human %>_t *)node)-><%= field.name %>.size; index++) {
-                if (index != 0) yp_buffer_append_str(buffer, ", ", 2);
-                prettyprint_location(buffer, parser, &((yp_<%= node.human %>_t *)node)-><%= field.name %>.locations[index]);
-            }
-            yp_buffer_append_str(buffer, "]", 1);
             <%- when YARP::ConstantField -%>
             char <%= field.name %>_buffer[12];
             snprintf(<%= field.name %>_buffer, sizeof(<%= field.name %>_buffer), "%u", ((yp_<%= node.human %>_t *)node)-><%= field.name %>);

diff --git a/templates/src/serialize.c.erb b/templates/src/serialize.c.erb
@@ -86,12 +86,6 @@ yp_serialize_node(yp_parser_t *parser, yp_node_t *node, yp_buffer_t *buffer) {
             for (uint32_t index = 0; index < <%= field.name %>_size; index++) {
                 yp_serialize_node(parser, (yp_node_t *) ((yp_<%= node.human %>_t *)node)-><%= field.name %>.nodes[index], buffer);
             }
-            <%- when YARP::LocationListField -%>
-            uint32_t <%= field.name %>_size = yp_sizet_to_u32(((yp_<%= node.human %>_t *)node)-><%= field.name %>.size);
-            yp_buffer_append_u32(buffer, <%= field.name %>_size);
-            for (uint32_t index = 0; index < <%= field.name %>_size; index++) {
-                yp_serialize_location(parser, &((yp_<%= node.human %>_t *)node)-><%= field.name %>.locations[index], buffer);
-            }
             <%- when YARP::ConstantField -%>
             yp_buffer_append_u32(buffer, yp_sizet_to_u32(((yp_<%= node.human %>_t *)node)-><%= field.name %>));
             <%- when YARP::ConstantListField -%>

diff --git a/templates/template.rb b/templates/template.rb
@@ -73,17 +73,6 @@ def java_type
     end
   end
 
-  # This represents a field on a node that is a list of locations.
-  class LocationListField < Field
-    def rbs_class
-      "Array[Location]"
-    end
-
-    def java_type
-      "Location[]"
-    end
-  end
-
   # This represents a field on a node that is the ID of a string interned
   # through the parser's constant pool.
   class ConstantField < Field
@@ -205,7 +194,6 @@ def field_type_for(name)
       when "node?"      then OptionalNodeField
       when "node[]"     then NodeListField
       when "string"     then StringField
-      when "location[]" then LocationListField
       when "constant"   then ConstantField
       when "constant[]" then ConstantListField
       when "location"   then LocationField