Re: Branch Review

Chip Salzenberg <chip@pobox.com> wrote:

[ sorry for the delayed answer ]

> I've looked over over the diffs between trunk and leo-ctx5, and here
> are my notes.

>{{ OVERALL }}

> A significant improvement.  Good work, y'all.

Thanks.

>{{ USER-VISIBLE }}

> * optional parameter interface: ":opt_count" -> ":opt_flag"

>       So ":opt_count" should be replaced with ":opt_flag", which sets
>       its target register to a boolean indicating whether the
>       immediately preceding parameter was passed or not, rather than a
>       count of optionals present.

If there is an alternating sequence of (:optional, :opt_count)+, the
:opt_count works exactly like :opt_flag, i.e. you get 0 or 1 depending
on the presence of the preceeding optional argument.

OTOH the current :opt_count is also usable to just count a bunch of
preceeding optionals. Therefore this proposed change would just reduce
the functionality of the current implementation.

> * subroutine attribute spelling: "method" -> ":method"

>            .sub "foo" :multi(_,int) :method

ACK. What about @MAIN, @IMMEDIATE, ...?

>       Of course, for the time being, "@MULTI" is a synonym for ":multi".

> * src/embed.c : setup_argv() - setting P5

>     + Looks like this function is still setting P5.  Hm.  (?)

Starting up :main could probably use Parrot_runops_fromc_args(... argv).
I'll have a look at it.

> * src/inter_run.c : runops_args() - argument limit

>     + Looks like this function fails if someone passes more than eight
>       parameters.  Why?  Buffer malloc is cheap.

There are more argument limits currently due to limited register range.
These will be fixed, when variable sized register frames are done.

> * DELETED opcode(s) can die

I'm ususally collecting a bunch of opcode deletes and actually remove
opcodes from *.ops and ops.num not to often. There are more outstanding
opcode removals (see also F<DEPRECATED>).
The reason is that deleting opcodes not only invalidates existing .PBCs
but also needs adjusting a lot of examples/japh/*

> * Tests may not use literal get/set flags

>     + The values of the flags for get_params/set_args/etc. should not
>       escape in numeric form, even to the self-tests.  If they're out
>       there in symbolic form, fine.

Yep. Presumably the (successor of the) macro preprocessor should handle
this.

> * struct Parrot_Context

>     + "ref_count"

>       Why are we going there?  Refcounting bad, reference observation
>       via GC good, right?  I may have totally missed the point of this.

The context structure is owned by continuations. Multiple continuations
can point to the same context structure. The ref_count of the context
holds the amount of active continuations refering to the context.
Continuations are subject to GC, as continuations are first class
objects. But the associated context can only be refered to by
continuations, therefore ref_count is neither bad nor unneeded.

> * classes/bound_nci.pmc

This PMC is obsolete and should be replaced by a general currying
scheme.

>       Update: On IRC, Leo describes bound_nci as "b0rken"; OK, but
>       it'd be better to nuke that code and replace it with a die and
>       comment as to what should happen.

Yep.

> * include/parrot/interpreter.h

>     + There are two different structures with "current_object".
>       Is this OK, or a surprise bug, a temporary hack, or ... ?

The 'current_object', which should better be 'current_invocant' is first
set in the interpreter (as there isn't yet a context before the call).
Then, if a PASM/PIR function is called, the information is copied into
the new context of that subroutine.

> * imcc/pbc.c

> - * Actually it doesn't verify anything, but rather, it _corrects_ it,

Sure. It verifies that no constants are used as out arguments. And it
fills in type bits. This is what the comment is stating.

> * pcc/get/args.c

>     + I think this comment speaks for itself:

>         +        strcat(buf, s);         /* XXX check avail len */

>       That's a security/coredump problem.  It can't wait until later.

Yep. As said, we need variable sized register frames first, to get rid
of length (and argument count) limitations. See also the disabled spill
tests (imcc/t/reg/spill.t).

> * documentation

I've tried to sync inline POD docs but might of course haved missed
some.

leo

• Branch Review by Chip Salzenberg
• Re: Branch Review by Leopold Toetsch
• Re: Branch Review by Chip Salzenberg
• Re: Branch Review by Leopold Toetsch
• Re: Branch Review by Leopold Toetsch