From 2c0842b3c57a1f2a6556690c6ca7c224d49144d8 Mon Sep 17 00:00:00 2001 From: Damien George Date: Sun, 27 Apr 2014 16:46:51 +0100 Subject: [PATCH] py: Change the way function arguments are compiled. New way uses slightly less ROM and RAM, should be slightly faster, and, most importantly, allows to catch the error "non-keyword arg following keyword arg". Addresses issue #466. --- py/compile.c | 133 +++++++++++++++++++++++---------------------------- py/grammar.h | 8 ++-- 2 files changed, 64 insertions(+), 77 deletions(-) diff --git a/py/compile.c b/py/compile.c index 2580cbeb51..6abe9ea2d7 100644 --- a/py/compile.c +++ b/py/compile.c @@ -51,8 +51,6 @@ typedef struct _compiler_t { int break_continue_except_level; uint16_t cur_except_level; // increased for SETUP_EXCEPT, SETUP_FINALLY; decreased for POP_BLOCK, POP_EXCEPT - uint16_t n_arg_keyword; - uint8_t star_flags; uint8_t have_star; uint16_t num_dict_params; uint16_t num_default_params; @@ -264,6 +262,7 @@ STATIC mp_parse_node_t fold_constants(mp_parse_node_t pn) { } STATIC void compile_trailer_paren_helper(compiler_t *comp, mp_parse_node_t pn_arglist, bool is_method_call, int n_positional_extra); +void compile_comprehension(compiler_t *comp, mp_parse_node_struct_t *pns, scope_kind_t kind); STATIC void compile_node(compiler_t *comp, mp_parse_node_t pn); STATIC uint comp_next_label(compiler_t *comp) { @@ -298,21 +297,6 @@ STATIC scope_t *scope_new_and_link(compiler_t *comp, scope_kind_t kind, mp_parse return scope; } -STATIC int list_len(mp_parse_node_t pn, int pn_kind) { - if (MP_PARSE_NODE_IS_NULL(pn)) { - return 0; - } else if (MP_PARSE_NODE_IS_LEAF(pn)) { - return 1; - } else { - mp_parse_node_struct_t *pns = (mp_parse_node_struct_t*)pn; - if (MP_PARSE_NODE_STRUCT_KIND(pns) != pn_kind) { - return 1; - } else { - return MP_PARSE_NODE_STRUCT_NUM_NODES(pns); - } - } -} - STATIC void apply_to_single_or_list(compiler_t *comp, mp_parse_node_t pn, int pn_list_kind, void (*f)(compiler_t*, mp_parse_node_t)) { if (MP_PARSE_NODE_IS_STRUCT(pn) && MP_PARSE_NODE_STRUCT_KIND((mp_parse_node_struct_t*)pn) == pn_list_kind) { mp_parse_node_struct_t *pns = (mp_parse_node_struct_t*)pn; @@ -485,7 +469,7 @@ STATIC void cpython_c_tuple(compiler_t *comp, mp_parse_node_t pn, mp_parse_node_ #endif // funnelling all tuple creations through this function is purely so we can optionally agree with CPython -void c_tuple(compiler_t *comp, mp_parse_node_t pn, mp_parse_node_struct_t *pns_list) { +STATIC void c_tuple(compiler_t *comp, mp_parse_node_t pn, mp_parse_node_struct_t *pns_list) { #if MICROPY_EMIT_CPYTHON cpython_c_tuple(comp, pn, pns_list); #else @@ -2331,30 +2315,70 @@ STATIC void compile_trailer_paren_helper(compiler_t *comp, mp_parse_node_t pn_ar } #endif - uint old_n_arg_keyword = comp->n_arg_keyword; - uint old_star_flags = comp->star_flags; - comp->n_arg_keyword = 0; - comp->star_flags = 0; + // get the list of arguments + mp_parse_node_t *args; + int n_args = list_get(&pn_arglist, PN_arglist, &args); - compile_node(comp, pn_arglist); // arguments to function call; can be null - - // compute number of positional arguments - int n_positional = n_positional_extra + list_len(pn_arglist, PN_arglist) - comp->n_arg_keyword; - if (comp->star_flags & MP_EMIT_STAR_FLAG_SINGLE) { - n_positional -= 1; - } - if (comp->star_flags & MP_EMIT_STAR_FLAG_DOUBLE) { - n_positional -= 1; + // compile the arguments + // Rather than calling compile_node on the list, we go through the list of args + // explicitly here so that we can count the number of arguments and give sensible + // error messages. + int n_positional = n_positional_extra; + uint n_keyword = 0; + uint star_flags = 0; + for (int i = 0; i < n_args; i++) { + if (MP_PARSE_NODE_IS_STRUCT(args[i])) { + mp_parse_node_struct_t *pns_arg = (mp_parse_node_struct_t*)args[i]; + if (MP_PARSE_NODE_STRUCT_KIND(pns_arg) == PN_arglist_star) { + if (star_flags & MP_EMIT_STAR_FLAG_SINGLE) { + compile_syntax_error(comp, (mp_parse_node_t)pns_arg, "can't have multiple *x"); + return; + } + star_flags |= MP_EMIT_STAR_FLAG_SINGLE; + compile_node(comp, pns_arg->nodes[0]); + } else if (MP_PARSE_NODE_STRUCT_KIND(pns_arg) == PN_arglist_dbl_star) { + if (star_flags & MP_EMIT_STAR_FLAG_DOUBLE) { + compile_syntax_error(comp, (mp_parse_node_t)pns_arg, "can't have multiple **x"); + return; + } + star_flags |= MP_EMIT_STAR_FLAG_DOUBLE; + compile_node(comp, pns_arg->nodes[0]); + } else if (MP_PARSE_NODE_STRUCT_KIND(pns_arg) == PN_argument) { + assert(MP_PARSE_NODE_IS_STRUCT(pns_arg->nodes[1])); // should always be + mp_parse_node_struct_t *pns2 = (mp_parse_node_struct_t*)pns_arg->nodes[1]; + if (MP_PARSE_NODE_STRUCT_KIND(pns2) == PN_argument_3) { + if (!MP_PARSE_NODE_IS_ID(pns_arg->nodes[0])) { + compile_syntax_error(comp, (mp_parse_node_t)pns_arg, "LHS of keyword arg must be an id"); + return; + } + EMIT_ARG(load_const_id, MP_PARSE_NODE_LEAF_ARG(pns_arg->nodes[0])); + compile_node(comp, pns2->nodes[0]); + n_keyword += 1; + } else { + assert(MP_PARSE_NODE_STRUCT_KIND(pns2) == PN_comp_for); // should always be + compile_comprehension(comp, pns_arg, SCOPE_GEN_EXPR); + n_positional++; + } + } else { + goto normal_argument; + } + } else { + normal_argument: + if (n_keyword > 0) { + compile_syntax_error(comp, args[i], "non-keyword arg after keyword arg"); + return; + } + compile_node(comp, args[i]); + n_positional++; + } } + // emit the function/method call if (is_method_call) { - EMIT_ARG(call_method, n_positional, comp->n_arg_keyword, comp->star_flags); + EMIT_ARG(call_method, n_positional, n_keyword, star_flags); } else { - EMIT_ARG(call_function, n_positional, comp->n_arg_keyword, comp->star_flags); + EMIT_ARG(call_function, n_positional, n_keyword, star_flags); } - - comp->n_arg_keyword = old_n_arg_keyword; - comp->star_flags = old_star_flags; } void compile_power_trailers(compiler_t *comp, mp_parse_node_struct_t *pns) { @@ -2677,43 +2701,6 @@ void compile_classdef(compiler_t *comp, mp_parse_node_struct_t *pns) { EMIT_ARG(store_id, cname); } -void compile_arglist_star(compiler_t *comp, mp_parse_node_struct_t *pns) { - if (comp->star_flags & MP_EMIT_STAR_FLAG_SINGLE) { - compile_syntax_error(comp, (mp_parse_node_t)pns, "can't have multiple *x"); - return; - } - comp->star_flags |= MP_EMIT_STAR_FLAG_SINGLE; - compile_node(comp, pns->nodes[0]); -} - -void compile_arglist_dbl_star(compiler_t *comp, mp_parse_node_struct_t *pns) { - if (comp->star_flags & MP_EMIT_STAR_FLAG_DOUBLE) { - compile_syntax_error(comp, (mp_parse_node_t)pns, "can't have multiple **x"); - return; - } - comp->star_flags |= MP_EMIT_STAR_FLAG_DOUBLE; - compile_node(comp, pns->nodes[0]); -} - -void compile_argument(compiler_t *comp, mp_parse_node_struct_t *pns) { - assert(MP_PARSE_NODE_IS_STRUCT(pns->nodes[1])); // should always be - mp_parse_node_struct_t *pns2 = (mp_parse_node_struct_t*)pns->nodes[1]; - if (MP_PARSE_NODE_STRUCT_KIND(pns2) == PN_argument_3) { - if (!MP_PARSE_NODE_IS_ID(pns->nodes[0])) { - compile_syntax_error(comp, (mp_parse_node_t)pns, "left-hand-side of keyword argument must be an id"); - return; - } - EMIT_ARG(load_const_id, MP_PARSE_NODE_LEAF_ARG(pns->nodes[0])); - compile_node(comp, pns2->nodes[0]); - comp->n_arg_keyword += 1; - } else if (MP_PARSE_NODE_STRUCT_KIND(pns2) == PN_comp_for) { - compile_comprehension(comp, pns, SCOPE_GEN_EXPR); - } else { - // shouldn't happen - assert(0); - } -} - void compile_yield_expr(compiler_t *comp, mp_parse_node_struct_t *pns) { if (comp->scope_cur->kind != SCOPE_FUNCTION && comp->scope_cur->kind != SCOPE_LAMBDA) { compile_syntax_error(comp, (mp_parse_node_t)pns, "'yield' outside function"); diff --git a/py/grammar.h b/py/grammar.h index 4823ccb12b..20c468298c 100644 --- a/py/grammar.h +++ b/py/grammar.h @@ -275,10 +275,10 @@ DEF_RULE(classdef_2, nc, and(3), tok(DEL_PAREN_OPEN), opt_rule(arglist), tok(DEL // arglist: (argument ',')* (argument [','] | '*' test (',' argument)* [',' '**' test] | '**' test) // TODO arglist lets through more than is allowed, compiler needs to do further verification -DEF_RULE(arglist, c(generic_all_nodes), list_with_end, rule(arglist_2), tok(DEL_COMMA)) +DEF_RULE(arglist, nc, list_with_end, rule(arglist_2), tok(DEL_COMMA)) DEF_RULE(arglist_2, nc, or(3), rule(arglist_star), rule(arglist_dbl_star), rule(argument)) -DEF_RULE(arglist_star, c(arglist_star), and(2), tok(OP_STAR), rule(test)) -DEF_RULE(arglist_dbl_star, c(arglist_dbl_star), and(2), tok(OP_DBL_STAR), rule(test)) +DEF_RULE(arglist_star, nc, and(2), tok(OP_STAR), rule(test)) +DEF_RULE(arglist_dbl_star, nc, and(2), tok(OP_DBL_STAR), rule(test)) // # The reason that keywords are test nodes instead of NAME is that using NAME // # results in an ambiguity. ast.c makes sure it's a NAME. @@ -287,7 +287,7 @@ DEF_RULE(arglist_dbl_star, c(arglist_dbl_star), and(2), tok(OP_DBL_STAR), rule(t // comp_for: 'for' exprlist 'in' or_test [comp_iter] // comp_if: 'if' test_nocond [comp_iter] -DEF_RULE(argument, c(argument), and(2), rule(test), opt_rule(argument_2)) +DEF_RULE(argument, nc, and(2), rule(test), opt_rule(argument_2)) DEF_RULE(argument_2, nc, or(2), rule(comp_for), rule(argument_3)) DEF_RULE(argument_3, nc, and(2), tok(DEL_EQUAL), rule(test)) DEF_RULE(comp_iter, nc, or(2), rule(comp_for), rule(comp_if))