On Wednesday 06 February 2008 16:33:11 [EMAIL PROTECTED] wrote:
> Modified:
> trunk/include/parrot/sub.h
> trunk/src/pmc/sub.pmc
>
> Log:
> [core] Implement inspect and inspect_str on the Sub PMC, making various
> bits of information about the arguments it takes available. Re-implement
> arity in terms of inspect_str. As a bonus, the data is now cached, so it'll
> perform better.
I'm very glad to see this. Here are a couple of minor style comments.
> --- trunk/src/pmc/sub.pmc (original)
> +++ trunk/src/pmc/sub.pmc Wed Feb 6 16:33:10 2008
> +=item C<PMC* inspect_str(STRING* what)>
The star should go on the variable name (at least for incoming parameters).
That way we avoid the problem of:
char* foo, bar; /* oops, bar isn't a pointer! */
> + {
> + opcode_t *pc;
> +
> + /* Allocate structure to store argument information in. */
> + sub->arg_info =
> mem_sys_allocate_zeroed(sizeof(Parrot_sub_arginfo));
The mem_sys_allocate_zeroed_typed() is slightly nicer here, and it's C++
friendly.
> +
> + /* Get pointer into the bytecode where this sub starts. */
> + pc = sub->seg->base.data + sub->start_offs;
I'd hoist the assignment of pc up to its declaration.
> + if (PARROT_ARG_SLURPY_ARRAY_ISSET(sig_item)){
> + if (PARROT_ARG_NAME_ISSET(sig_item))
> + sub->arg_info->named_slurpy = 1;
> + else
> + sub->arg_info->pos_slurpy = 1;
> + }
> + else if (PARROT_ARG_OPTIONAL_ISSET(sig_item)) {
> + if (PARROT_ARG_NAME_ISSET(sig_item))
> + sub->arg_info->named_optional++;
> + else
> + sub->arg_info->pos_optional++;
> + }
> + else if (!PARROT_ARG_OPT_FLAG_ISSET(sig_item)) {
> + if (PARROT_ARG_NAME_ISSET(sig_item))
> + sub->arg_info->named_required++;
> + else
> + sub->arg_info->pos_required++;
> + }
This logic confuses me slightly. What's the difference between OPTIONAL and
OPT_FLAG?
> + /* Return the argument information that was requested. */
> + if (string_equal(interp, what, CONST_STRING(interp,
> "pos_required")) == 0) {
The STR_EQ macro simplifies this slightly.
> + if (count_found >= 0) {
> + PMC *retval = pmc_new(INTERP, enum_class_Integer);
> + VTABLE_set_integer_native(INTERP, retval, count_found);
> + return retval;
> + }
> + else {
> + real_exception(interp, NULL, INVALID_OPERATION,
> + "Unknown introspection value '%S'", what);
> + return PMCNULL;
> + }
> + }
The return here is unreachable. You can probably get rid of the else clause
and one level of nesting and put the exception as the last part of the code.
That way it may be clearer that there's one and only one good way out of this
function.
-- c
