Skip to content

Commit

Permalink
Add line_count field to rb_ast_body_t
Browse files Browse the repository at this point in the history
This patch adds `int line_count` field to `rb_ast_body_t` structure.
Instead, we no longer cast `script_lines` to Fixnum.

## Background

Ref #10618

In the PR above, we have decoupled IMEMO from `rb_ast_t`.
This means we could lift the five-words-restriction of the structure
that forced us to unionize `rb_ast_t *` and `FIXNUM` in one field.

## Relating refactor

- Remove the second parameter of `rb_ruby_ast_new()` function

## Attention

I will remove a code that assigns -1 to line_count, in `rb_binding_add_dynavars()`
of vm.c, because I don't think it is necessary.
But I will make another PR for this so that we can atomically revert
in case I was wrong (See the comment on the code)
  • Loading branch information
hasumikin authored and yui-knk committed Apr 27, 2024
1 parent bf1f16e commit 55a402b
Show file tree
Hide file tree
Showing 9 changed files with 29 additions and 30 deletions.
1 change: 0 additions & 1 deletion ast.c
Expand Up @@ -802,7 +802,6 @@ ast_node_script_lines(rb_execution_context_t *ec, VALUE self)
TypedData_Get_Struct(self, struct ASTNodeData, &rb_node_type, data);
ast = rb_ruby_ast_data_get(data->vast);
rb_parser_ary_t *ret = ast->body.script_lines;
if (!ret || FIXNUM_P((VALUE)ret)) return Qnil;
return rb_parser_build_script_lines_from(ret);
}

Expand Down
4 changes: 2 additions & 2 deletions compile.c
Expand Up @@ -1479,7 +1479,7 @@ new_child_iseq(rb_iseq_t *iseq, const NODE *const node,
VALUE name, const rb_iseq_t *parent, enum rb_iseq_type type, int line_no)
{
rb_iseq_t *ret_iseq;
VALUE vast = rb_ruby_ast_new(node, NULL);
VALUE vast = rb_ruby_ast_new(node);

debugs("[new_child_iseq]> ---------------------------------------\n");
int isolated_depth = ISEQ_COMPILE_DATA(iseq)->isolated_depth;
Expand Down Expand Up @@ -8771,7 +8771,7 @@ compile_builtin_mandatory_only_method(rb_iseq_t *iseq, const NODE *node, const N
scope_node.nd_body = mandatory_node(iseq, node);
scope_node.nd_args = &args_node;

VALUE vast = rb_ruby_ast_new(RNODE(&scope_node), NULL);
VALUE vast = rb_ruby_ast_new(RNODE(&scope_node));

ISEQ_BODY(iseq)->mandatory_only_iseq =
rb_iseq_new_with_opt(vast, rb_iseq_base_label(iseq),
Expand Down
2 changes: 1 addition & 1 deletion internal/ruby_parser.h
Expand Up @@ -96,7 +96,7 @@ enum lex_state_e {
EXPR_NONE = 0
};

VALUE rb_ruby_ast_new(const NODE *const root, rb_parser_ary_t *script_lines);
VALUE rb_ruby_ast_new(const NODE *const root);
rb_ast_t *rb_ruby_ast_data_get(VALUE vast);

#endif /* INTERNAL_RUBY_PARSE_H */
11 changes: 2 additions & 9 deletions iseq.c
Expand Up @@ -851,14 +851,7 @@ static int
ast_line_count(const VALUE vast)
{
rb_ast_t *ast = rb_ruby_ast_data_get(vast);
if (!ast || !ast->body.script_lines) {
// this occurs when failed to parse the source code with a syntax error
return 0;
}
if (!FIXNUM_P((VALUE)ast->body.script_lines)) {
return (int)ast->body.script_lines->len;
}
return FIX2INT((VALUE)ast->body.script_lines);
return ast->body.line_count;
}

static VALUE
Expand Down Expand Up @@ -999,7 +992,7 @@ rb_iseq_new_with_opt(const VALUE vast, VALUE name, VALUE path, VALUE realpath,
if (!NIL_P(script_lines)) {
// noop
}
else if (body && !FIXNUM_P((VALUE)body->script_lines) && body->script_lines) {
else if (body && body->script_lines) {
script_lines = rb_parser_build_script_lines_from(body->script_lines);
}
else if (parent) {
Expand Down
15 changes: 6 additions & 9 deletions node.c
Expand Up @@ -340,6 +340,7 @@ iterate_node_values(rb_ast_t *ast, node_buffer_list_t *nb, node_itr_t * func, vo
static void
script_lines_free(rb_ast_t *ast, rb_parser_ary_t *script_lines)
{
if (!script_lines) return;
for (long i = 0; i < script_lines->len; i++) {
parser_string_free(ast, (rb_parser_string_t *)script_lines->data[i]);
}
Expand All @@ -358,10 +359,8 @@ rb_ast_free(rb_ast_t *ast)
#ifdef UNIVERSAL_PARSER
if (ast && ast->node_buffer) {
void (*free_func)(void *) = xfree;
if (ast->body.script_lines && !FIXNUM_P((VALUE)ast->body.script_lines)) {
script_lines_free(ast, ast->body.script_lines);
ast->body.script_lines = NULL;
}
script_lines_free(ast, ast->body.script_lines);
ast->body.script_lines = NULL;
rb_node_buffer_free(ast, ast->node_buffer);
ast->node_buffer = 0;
free_func(ast);
Expand Down Expand Up @@ -418,7 +417,7 @@ rb_ast_memsize(const rb_ast_t *ast)
}
}

if (script_lines && !FIXNUM_P((VALUE)script_lines)) {
if (script_lines) {
size += sizeof(rb_parser_ary_t);
for (i = 0; i < script_lines->len; i++) {
size += sizeof(rb_parser_string_t);
Expand All @@ -436,10 +435,8 @@ rb_ast_dispose(rb_ast_t *ast)
// noop. See the comment in rb_ast_free().
#else
if (ast && ast->node_buffer) {
if (ast->body.script_lines && !FIXNUM_P((VALUE)ast->body.script_lines)) {
script_lines_free(ast, ast->body.script_lines);
ast->body.script_lines = NULL;
}
script_lines_free(ast, ast->body.script_lines);
ast->body.script_lines = NULL;
rb_node_buffer_free(ast, ast->node_buffer);
ast->node_buffer = 0;
}
Expand Down
2 changes: 1 addition & 1 deletion parse.y
Expand Up @@ -7755,7 +7755,7 @@ yycompile0(VALUE arg)
}
}
p->ast->body.root = tree;
if (!p->ast->body.script_lines) p->ast->body.script_lines = (rb_parser_ary_t *)INT2FIX(p->line_count);
p->ast->body.line_count = p->line_count;
return TRUE;
}

Expand Down
8 changes: 5 additions & 3 deletions ruby_parser.c
Expand Up @@ -893,6 +893,7 @@ VALUE
rb_parser_build_script_lines_from(rb_parser_ary_t *lines)
{
int i;
if (!lines) return Qnil;
if (lines->data_type != PARSER_ARY_DATA_SCRIPT_LINE) {
rb_bug("unexpected rb_parser_ary_data_type (%d) for script lines", lines->data_type);
}
Expand Down Expand Up @@ -1115,7 +1116,7 @@ parser_aset_script_lines_for(VALUE path, rb_parser_ary_t *lines)
{
VALUE hash, script_lines;
ID script_lines_id;
if (NIL_P(path) || !lines || FIXNUM_P((VALUE)lines)) return;
if (NIL_P(path) || !lines) return;
CONST_ID(script_lines_id, "SCRIPT_LINES__");
if (!rb_const_defined_at(rb_cObject, script_lines_id)) return;
hash = rb_const_get_at(rb_cObject, script_lines_id);
Expand All @@ -1126,15 +1127,16 @@ parser_aset_script_lines_for(VALUE path, rb_parser_ary_t *lines)
}

VALUE
rb_ruby_ast_new(const NODE *const root, rb_parser_ary_t *script_lines)
rb_ruby_ast_new(const NODE *const root)
{
VALUE vast = ast_alloc();
rb_ast_t *ast = DATA_PTR(vast);
ast->body = (rb_ast_body_t){
.root = root,
.frozen_string_literal = -1,
.coverage_enabled = -1,
.script_lines = script_lines
.script_lines = NULL,
.line_count = 0,
};
return vast;
}
Expand Down
4 changes: 1 addition & 3 deletions rubyparser.h
Expand Up @@ -1212,9 +1212,7 @@ typedef struct node_buffer_struct node_buffer_t;
typedef struct rb_ast_body_struct {
const NODE *root;
rb_parser_ary_t *script_lines;
// script_lines is either:
// - a Fixnum that represents the line count of the original source, or
// - an rb_parser_ary_t* that contains the lines of the original source
int line_count;
signed int frozen_string_literal:2; /* -1: not specified, 0: false, 1: true */
signed int coverage_enabled:2; /* -1: not specified, 0: false, 1: true */
} rb_ast_body_t;
Expand Down
12 changes: 11 additions & 1 deletion vm.c
Expand Up @@ -1480,7 +1480,17 @@ rb_binding_add_dynavars(VALUE bindval, rb_binding_t *bind, int dyncount, const I
tmp_node.nd_body = 0;
tmp_node.nd_args = 0;

VALUE vast = rb_ruby_ast_new(RNODE(&tmp_node), (rb_parser_ary_t *)INT2FIX(-1));
VALUE vast = rb_ruby_ast_new(RNODE(&tmp_node));
{ /*
* TODO:
* Assigning -1 to line_count is to maintain the previous code.
* However, the author of this patch guesses this code is no longer necessary.
* We will try to remove this code in the next commit which is atomically
* revertable if the author is wrong.
*/
rb_ast_t *ast = (rb_ast_t *)DATA_PTR(vast);
ast->body.line_count = -1;
}

if (base_iseq) {
iseq = rb_iseq_new(vast, ISEQ_BODY(base_iseq)->location.label, path, realpath, base_iseq, ISEQ_TYPE_EVAL);
Expand Down

0 comments on commit 55a402b

Please sign in to comment.