From 0995739ca8815446d1548bc02ef013df36ae0791 Mon Sep 17 00:00:00 2001 From: Laria Carolin Chabowski Date: Sat, 18 Dec 2021 21:39:48 +0100 Subject: [PATCH] Parser: Do a better job at cleaning up temporary allocations -fsanitize=memory no longer finds any memory leak in the parser test suite. We don't yet test error cases, but still pretty good :). --- src/expr.c | 1 + src/parser.c | 196 +++++++++++++++++++++++++++++++-------------------- 2 files changed, 120 insertions(+), 77 deletions(-) diff --git a/src/expr.c b/src/expr.c index dc18a3e..4964a60 100644 --- a/src/expr.c +++ b/src/expr.c @@ -73,6 +73,7 @@ void apfl_expr_call_deinit(struct apfl_expr_call *call) { DESTROY(call->callee, apfl_expr_deinit); + apfl_expr_list_deinit(&call->arguments); } void diff --git a/src/parser.c b/src/parser.c index 263aafc..f1a5338 100644 --- a/src/parser.c +++ b/src/parser.c @@ -138,7 +138,7 @@ fragment_deinit(struct fragment *fragment) deinit_fragment_lhs_rhs(&fragment->at); break; case FRAG_PREDICATE: - deinit_fragment_lhs_rhs(&fragment->at); + deinit_fragment_lhs_rhs(&fragment->predicate); break; case FRAG_EXPR: apfl_expr_deinit(&fragment->expr); @@ -373,7 +373,7 @@ parse_fragment_into_list(apfl_parser_ptr p, struct fragment_list *list, bool nee struct fragment *elem = &list->children[list->len]; enum parse_fragment_result result = parse_fragment(p, elem, need, flags); - if (result != PF_OK) {// \mystuff\TODO:even more flags? + if (result != PF_OK) { return result; } @@ -393,7 +393,7 @@ err_unexpected_eof_after(enum apfl_token_type token_type, struct apfl_position p static bool fragments_to_call( apfl_parser_ptr, - struct fragment_list *, + struct fragment_list, struct apfl_position position, struct apfl_expr * ); @@ -465,7 +465,7 @@ parse_parens(apfl_parser_ptr p, struct fragment *out, struct apfl_position posit out->type = FRAG_EXPR; out->position = position; - if (!fragments_to_call(p, &children, position, &out->expr)) { + if (!fragments_to_call(p, children, position, &out->expr)) { goto error; } @@ -569,62 +569,60 @@ static bool fragment_to_list_item( static struct apfl_expr *fragment_to_expr_allocated(apfl_parser_ptr, struct fragment); static bool -fragment_to_expr(apfl_parser_ptr p, struct fragment fragment, struct apfl_expr *out) +fragment_to_expr_inner(apfl_parser_ptr p, struct fragment *fragment, struct apfl_expr *out) { - switch (fragment.type) { + switch (fragment->type) { case FRAG_EXPAND: - p->error = err_unexpected_token(APFL_TOK_EXPAND, fragment.position); + p->error = err_unexpected_token(APFL_TOK_EXPAND, fragment->position); return false; case FRAG_CONSTANT: out->type = APFL_EXPR_CONSTANT; - out->constant = fragment.constant; - out->position = fragment.position; + out->constant = apfl_expr_const_move(&fragment->constant); + out->position = fragment->position; return true; case FRAG_NAME: out->type = APFL_EXPR_VAR; - out->var = apfl_string_move(&fragment.name); - out->position = fragment.position; + out->var = apfl_string_move(&fragment->name); + out->position = fragment->position; return true; case FRAG_DOT: out->type = APFL_EXPR_DOT; - if ((out->dot.lhs = fragment_to_expr_allocated(p, fragment_move(fragment.dot.lhs))) == NULL) { + if ((out->dot.lhs = fragment_to_expr_allocated(p, fragment_move(fragment->dot.lhs))) == NULL) { return false; } - out->dot.rhs = apfl_string_move(&fragment.dot.rhs); - out->position = fragment.position; + out->dot.rhs = apfl_string_move(&fragment->dot.rhs); + out->position = fragment->position; return true; case FRAG_AT: out->type = APFL_EXPR_AT; - if ((out->at.lhs = fragment_to_expr_allocated(p, fragment_move(fragment.at.lhs))) == NULL) { + if ((out->at.lhs = fragment_to_expr_allocated(p, fragment_move(fragment->at.lhs))) == NULL) { return false; } - if ((out->at.rhs = fragment_to_expr_allocated(p, fragment_move(fragment.at.rhs))) == NULL) { + if ((out->at.rhs = fragment_to_expr_allocated(p, fragment_move(fragment->at.rhs))) == NULL) { return false; } - out->position = fragment.position; + out->position = fragment->position; return true; case FRAG_PREDICATE: - p->error = err_unexpected_token(APFL_TOK_QUESTION_MARK, fragment.position); + p->error = err_unexpected_token(APFL_TOK_QUESTION_MARK, fragment->position); return false; case FRAG_EXPR: - *out = apfl_expr_move(&fragment.expr); + *out = apfl_expr_move(&fragment->expr); return true; case FRAG_LIST: out->type = APFL_EXPR_LIST; - out->position = fragment.position; + out->position = fragment->position; out->list.len = 0; - if (fragment.list.len == 0) { + if (fragment->list.len == 0) { out->list.items = NULL; return true; } - if ((out->list.items = ALLOC_LIST(struct apfl_expr_list_item, fragment.list.len)) == NULL) { + if ((out->list.items = ALLOC_LIST(struct apfl_expr_list_item, fragment->list.len)) == NULL) { p->error = apfl_error_simple(APFL_ERR_MALLOC_FAILED); - fragment_deinit(&fragment); return false; } - for (size_t i = 0; i < fragment.list.len; i++) { - if (!fragment_to_list_item(p, fragment_move(&fragment.list.children[i]), &out->list.items[i])) { - fragment_deinit(&fragment); + for (size_t i = 0; i < fragment->list.len; i++) { + if (!fragment_to_list_item(p, fragment_move(&fragment->list.children[i]), &out->list.items[i])) { return false; } out->list.len++; @@ -635,6 +633,14 @@ fragment_to_expr(apfl_parser_ptr p, struct fragment fragment, struct apfl_expr * assert(false); } +static bool +fragment_to_expr(apfl_parser_ptr p, struct fragment fragment, struct apfl_expr *out) +{ + bool result = fragment_to_expr_inner(p, &fragment, out); + fragment_deinit(&fragment); + return result; +} + static struct apfl_expr * fragment_to_expr_allocated(apfl_parser_ptr p, struct fragment fragment) { @@ -801,6 +807,7 @@ parse_list( if (!append_fragment(&list, first)) { fragment_deinit(&first); p->error = apfl_error_simple(APFL_ERR_MALLOC_FAILED); + fragment_list_deinit(&list); return false; } @@ -836,7 +843,7 @@ maybe_end: return true; error: - // \mystuff\TODO:clean up list + fragment_list_deinit(&list); return false; } @@ -965,7 +972,7 @@ parse_stringify(apfl_parser_ptr p, struct fragment *fragment, struct apfl_positi static bool fragment_to_param_recursive(apfl_parser_ptr, struct fragment *, struct apfl_expr_param *); -static bool fragments_to_params(apfl_parser_ptr, struct fragment_list *, struct apfl_expr_params *); +static bool fragments_to_params(apfl_parser_ptr, struct fragment_list, struct apfl_expr_params *); static bool predicate_fragment_to_param( @@ -1033,7 +1040,7 @@ fragment_to_param_recursive( return false; case FRAG_LIST: out->type = APFL_EXPR_PARAM_LIST; - return fragments_to_params(p, &fragment->list, &out->list); + return fragments_to_params(p, fragment_list_move(&fragment->list), &out->list); } assert(false); @@ -1064,28 +1071,28 @@ fragment_to_param( static bool fragments_to_params( apfl_parser_ptr p, - struct fragment_list *fragments, + struct fragment_list fragments, struct apfl_expr_params *out ) { bool result = true; - if (fragments->len == 0) { + if (fragments.len == 0) { out->len = 0; out->params = NULL; goto ok; } - out->params = ALLOC_LIST(struct apfl_expr_param, fragments->len); + out->params = ALLOC_LIST(struct apfl_expr_param, fragments.len); out->len = 0; if (out->params == NULL) { goto error; } bool seen_expand = false; - for (size_t i = 0; i < fragments->len; i++) { + for (size_t i = 0; i < fragments.len; i++) { if (!fragment_to_param( p, - &fragments->children[i], + &fragments.children[i], &out->params[i], &seen_expand )) { @@ -1101,7 +1108,7 @@ error: result = false; apfl_expr_params_deinit(out); ok: - fragment_list_deinit(fragments); + fragment_list_deinit(&fragments); return result; } @@ -1111,13 +1118,21 @@ fragment_to_assignable( bool expand_ok, struct fragment fragment, struct apfl_expr_assignable *out +); + +static bool +fragment_to_assignable_inner( + apfl_parser_ptr p, + bool expand_ok, + struct fragment *fragment, + struct apfl_expr_assignable *out ) { - switch (fragment.type) { + switch (fragment->type) { case FRAG_EXPAND: if (!expand_ok) { p->error = (struct apfl_error) { .type = APFL_ERR_INVALID_ASSIGNMENT_LHS, - .position = fragment.position, + .position = fragment->position, }; goto error; } @@ -1131,7 +1146,7 @@ fragment_to_assignable( if (!fragment_to_assignable( p, false, - fragment_move(fragment.expand), + fragment_move(fragment->expand), out->expand )) { goto error; @@ -1140,16 +1155,16 @@ fragment_to_assignable( return true; case FRAG_CONSTANT: out->type = APFL_EXPR_ASSIGNABLE_CONSTANT; - out->constant = apfl_expr_const_move(&fragment.constant); + out->constant = apfl_expr_const_move(&fragment->constant); return true; case FRAG_NAME: out->type = APFL_EXPR_ASSIGNABLE_VAR; - out->var = apfl_string_move(&fragment.name); + out->var = apfl_string_move(&fragment->name); return true; case FRAG_DOT: out->type = APFL_EXPR_ASSIGNABLE_DOT; - out->dot.rhs = apfl_string_move(&fragment.dot.rhs); + out->dot.rhs = apfl_string_move(&fragment->dot.rhs); if ((out->dot.lhs = malloc(sizeof(struct apfl_expr_assignable))) == NULL) { p->error = apfl_error_simple(APFL_ERR_MALLOC_FAILED); @@ -1159,7 +1174,7 @@ fragment_to_assignable( if (!fragment_to_assignable( p, expand_ok, - fragment_move(fragment.dot.lhs), + fragment_move(fragment->dot.lhs), out->dot.lhs )) { goto error; @@ -1180,7 +1195,7 @@ fragment_to_assignable( if (!fragment_to_assignable( p, expand_ok, - fragment_move(fragment.at.lhs), + fragment_move(fragment->at.lhs), out->at.lhs )) { free(out->at.rhs); @@ -1190,7 +1205,7 @@ fragment_to_assignable( if (!fragment_to_expr( p, - fragment_move(fragment.at.rhs), + fragment_move(fragment->at.rhs), out->at.rhs )) { goto error; @@ -1211,7 +1226,7 @@ fragment_to_assignable( if (!fragment_to_assignable( p, expand_ok, - fragment_move(fragment.at.lhs), + fragment_move(fragment->at.lhs), out->predicate.lhs )) { free(out->predicate.rhs); @@ -1221,7 +1236,7 @@ fragment_to_assignable( if (!fragment_to_expr( p, - fragment_move(fragment.at.rhs), + fragment_move(fragment->at.rhs), out->predicate.rhs )) { goto error; @@ -1231,22 +1246,22 @@ fragment_to_assignable( case FRAG_EXPR: p->error = (struct apfl_error) { .type = APFL_ERR_INVALID_ASSIGNMENT_LHS, - .position = fragment.position, + .position = fragment->position, }; goto error; case FRAG_LIST: out->type = APFL_EXPR_ASSIGNABLE_LIST; out->list.len = 0; - out->list.children = malloc(sizeof(struct apfl_expr_assignable) * fragment.list.len); + out->list.children = malloc(sizeof(struct apfl_expr_assignable) * fragment->list.len); expand_ok = true; - for (size_t i = 0; i < fragment.list.len; i++) { + for (size_t i = 0; i < fragment->list.len; i++) { struct apfl_expr_assignable *cur = &out->list.children[i]; if (!fragment_to_assignable( p, expand_ok, - fragment_move(&fragment.list.children[i]), + fragment_move(&fragment->list.children[i]), cur )) { goto error; @@ -1267,6 +1282,18 @@ error: return false; } +static bool +fragment_to_assignable( + apfl_parser_ptr p, + bool expand_ok, + struct fragment fragment, + struct apfl_expr_assignable *out +) { + bool result = fragment_to_assignable_inner(p, expand_ok, &fragment, out); + fragment_deinit(&fragment); + return result; +} + struct partial_assignment { struct apfl_expr_assignable lhs; bool local; @@ -1311,11 +1338,11 @@ fragment_to_list_item( static bool fragments_to_call( apfl_parser_ptr p, - struct fragment_list *fragments, // \mystuff\TODO:really a pointer? Why? + struct fragment_list fragments, struct apfl_position position, struct apfl_expr *out ) { - assert(fragments->len > 0); // \mystuff\TODO: Or should we check this here? + assert(fragments.len > 0); out->type = APFL_EXPR_CALL; out->position = position; @@ -1324,28 +1351,31 @@ fragments_to_call( .len = 0, }; - out->call.callee = fragment_to_expr_allocated(p, fragment_move(&fragments->children[0])); + out->call.callee = fragment_to_expr_allocated(p, fragment_move(&fragments.children[0])); if (out->call.callee == NULL) { goto error; } - if (fragments->len == 1) { + if (fragments.len == 1) { + fragment_list_deinit(&fragments); return true; } - out->call.arguments.items = ALLOC_LIST(struct apfl_expr_list_item, (fragments->len - 1)); + out->call.arguments.items = ALLOC_LIST(struct apfl_expr_list_item, (fragments.len - 1)); - for (size_t i = 1; i < fragments->len; i++) { - if (!fragment_to_list_item(p, fragment_move(&fragments->children[i]), &out->call.arguments.items[i-1])) { + for (size_t i = 1; i < fragments.len; i++) { + if (!fragment_to_list_item(p, fragment_move(&fragments.children[i]), &out->call.arguments.items[i-1])) { goto error; } out->call.arguments.len++; } + fragment_list_deinit(&fragments); return true; error: + fragment_list_deinit(&fragments); apfl_expr_deinit(out); return false; } @@ -1353,11 +1383,11 @@ error: static enum parse_body_or_toplevel_finalize_result parse_body_or_toplevel_finalize( apfl_parser_ptr p, - struct fragment_list *fragments, + struct fragment_list fragments, struct partial_assignment_list partial_assignments, struct apfl_expr *out ) { - if (fragments->len == 0) { + if (fragments.len == 0) { if (partial_assignments.len > 0) { p->error = (struct apfl_error) { .type = APFL_ERR_EMPTY_ASSIGNMENT, @@ -1366,6 +1396,8 @@ parse_body_or_toplevel_finalize( goto error; } + fragment_list_deinit(&fragments); + DEINIT_LIST(partial_assignments.items, partial_assignments.len, partial_assignment_deinit); return BODY_FINALIZE_EMPTY; } @@ -1383,22 +1415,25 @@ parse_body_or_toplevel_finalize( dest = dest->assignment.rhs; } - if (fragments->len == 1) { - if (!fragment_to_expr(p, fragment_move(&fragments->children[0]), dest)) { + DEINIT_LIST(partial_assignments.items, partial_assignments.len, partial_assignment_deinit); + + if (fragments.len == 1) { + if (!fragment_to_expr(p, fragment_move(&fragments.children[0]), dest)) { goto error; } } else { - if (!fragments_to_call(p, fragments, fragments->children[0].position, dest)) { + struct apfl_position position = fragments.children[0].position; + if (!fragments_to_call(p, fragment_list_move(&fragments), position, dest)) { goto error; } } - fragment_list_deinit(fragments); - DEINIT_LIST(partial_assignments.items, partial_assignments.len, partial_assignment_deinit); + fragment_list_deinit(&fragments); return BODY_FINALIZE_OK; error: + fragment_list_deinit(&fragments); DEINIT_LIST(partial_assignments.items, partial_assignments.len, partial_assignment_deinit); apfl_expr_deinit(out); return BODY_FINALIZE_ERROR; @@ -1437,7 +1472,7 @@ parse_body_or_toplevel( if (handle_eof) { switch (parse_body_or_toplevel_finalize( p, - fragments, + fragment_list_move(fragments), partial_assignment_list_move(&partial_assignments), out )) { @@ -1502,6 +1537,7 @@ break_inner: fragment_list_deinit(fragments); // Reset fragment list if (!fragment_to_assignable(p, false, fragment, &cur_partial->lhs)) { + fragment_deinit(&fragment); goto error; } @@ -1516,7 +1552,7 @@ break_inner: case APFL_TOK_SEMICOLON: switch (parse_body_or_toplevel_finalize( p, - fragments, + fragment_list_move(fragments), partial_assignment_list_move(&partial_assignments), out )) { @@ -1609,6 +1645,8 @@ parse_braces( switch (p->token.type) { case APFL_TOK_RBRACE: + fragment_list_deinit(&fragments); + // TODO: Rather fulgly duplication if (has_params) { // Finalize previous subfunc and append @@ -1618,8 +1656,8 @@ parse_braces( &subfuncs_len, &subfuncs_cap, &(struct apfl_expr_subfunc) { - .params = params, - .body = body, + .params = apfl_expr_params_move(¶ms), + .body = apfl_expr_body_move(&body), }, 1 )) { @@ -1652,8 +1690,6 @@ parse_braces( body_cap = 0; } - DEINIT_LIST(fragments.children, fragments.len, fragment_deinit); - return true; case APFL_TOK_MAPSTO: if (body.len > 0 && !has_params) { @@ -1672,8 +1708,8 @@ parse_braces( &subfuncs_len, &subfuncs_cap, &(struct apfl_expr_subfunc) { - .params = params, - .body = body, + .params = apfl_expr_params_move(¶ms), + .body = apfl_expr_body_move(&body), }, 1 )) { @@ -1686,7 +1722,7 @@ parse_braces( body_cap = 0; } - if (!fragments_to_params(p, &fragments, ¶ms)) { + if (!fragments_to_params(p, fragment_list_move(&fragments), ¶ms)) { goto error; } has_params = true; @@ -1702,7 +1738,7 @@ parse_braces( } error: - // \mystuff\TODO:cleanup + fragment_list_deinit(&fragments); return false; } @@ -1891,7 +1927,7 @@ apfl_parser_next(apfl_parser_ptr p) apfl_expr_deinit(&p->expr); } - struct fragment_list fragments; // TODO: Clean me up! + struct fragment_list fragments; apfl_resizable_init(APFL_RESIZABLE_ARGS(fragments, children)); switch (parse_body_or_toplevel( @@ -1903,18 +1939,24 @@ apfl_parser_next(apfl_parser_ptr p) )) { case PF_OK: p->has_expr = true; + fragment_list_deinit(&fragments); return APFL_PARSE_OK; case PF_ERROR: - return APFL_PARSE_ERROR; + goto error; case PF_EOF: + fragment_list_deinit(&fragments); return APFL_PARSE_EOF; case PF_CANT_HANDLE: read_token_after_cant_handle(p); p->error = ERR_UNEXPECTED_TOKEN(p->token); - return APFL_PARSE_ERROR; + goto error; } assert(false); + +error: + fragment_list_deinit(&fragments); + return APFL_PARSE_ERROR; } struct apfl_error