Branch Review

{{ OVERALL }}

A significant improvement. Good work, y'all.

{{ USER-VISIBLE }}

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

+ As some of you may recall, ":opt_count" semantics don't work if
we ever support named parameters ... and given that p6l may be
moving toward making P6 named parameters less insane[*], this is
not impossible for Parrot 1.1 or something.

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 you want to do this after the merge, OK, but please don't
release with ":opt_count".

I also had some idea of how to spell this more compactly, but
":opt_flag" has to work first, so let's start with that.

This needs to be fixed before merge, to minimize PIR language
shift for users.

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

+ Some time ago, in a conversation perhaps lost to time, I pointed
to ':method', not 'method', as the way method tags should look
on sub definitions. No commas, just ':method' and, at some
point (if not yet), ':multi':

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

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. (?)

* 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.

* DELETED opcode(s) can die

+ The changes to the invoke opcodes are binary-incompatible, which
I have NO PROBLEM with, but as long as we're breaking things, you
might as well get rid of "DELETED" opcodes, like "DELETED_invoke".

* 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. Just not as numbers. I want to
be able to change those numbers without causing errors; and more
to the point, I want to make sure that the tests that use a given
feature fail if that feature is removed.

The tests that use literal numeric get/set flag values should be
using PIR instead, I suppose. Or the syntax should be extended
to allow symbolic values instead of numbers. Whatever. The
point is that this, from pmc/mmd.t, should not exist:

get_params "(0,0,0x20,0x40)", P5, I5, P6, I7

{{ INTERNAL }}

* 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.

* classes/bound_nci.pmc

+ The invoke() method is still setting P2, but P2 isn't supposed
to be special any more.

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.

* include/parrot/interpreter.h

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

* imcc/pbc.c

+ A comment has been removed from verify_signature, but it still
applies, and should be restored. (Please don't remove 'todo'
comments without discussion.)

- * Actually it doesn't verify anything, but rather, it _corrects_ it,
- * silently. Which needs to change. XXX CHS

* 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.

* documentation

+ I'm not asking you to go back and add docs for everything -- not
that I'd mind! -- but when you touch a function so that its API
changes (and creating a subroutine where none had existed counts
as an API change), the function needs to be (re)documented.
It's just a matter of making sure everyone knows what it's
supposed to do and how to call it.

[*] I think we can all agree that it would be impossible to make them
_more_ insane.
--
Chip Salzenberg <ch...@pobox.com>

[ sorry for the delayed answer ]

>{{ OVERALL }}

Thanks.

>{{ USER-VISIBLE }}

> So ":opt_count" should be replaced with ":opt_flag", which sets

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.

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

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

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

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

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/*

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

> * struct Parrot_Context

> + "ref_count"

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.

Yep.

> * include/parrot/interpreter.h

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

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

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

I don't know how you did it, but you have figured out my brilliant
plan. I've been intending all along to disable more and more features
of Parrot until it becomes a subset of JVM. At that point we could
have imported Kaffe and declared victory. I would have succeeded,
too, if not for those meddling kids!

But seriously: I've been asking you to remove the counting feature
for, what, months now? And for a consistent reason: Future
development of the parameter passing interface will likely make it
inconvenient (read 'inefficient') to implement. And the current user
base (pretty much Patrick and the tcl guys) have said they don't mind
switching to a boolean.

----------------------------------------------------------------

Same deal: the colon form will be standard, but the @ form should be
accepted until users have switched (or a few months, whichever comes
first). It's a totally minor point, I know, but I have a weakness for
the perl 5 modifier syntax.

("Everybody wants the colon", yeah, I know.)

----------------------------------------------------------------

OK, but please add a FIXME comment.

----------------------------------------------------------------

> The reason [for delay] is that deleting opcodes not only invalidates

There's no rush of course on the deletions, but I didn't think opcode
renumbering could break pasm code. I'd like to understand the interaction.

----------------------------------------------------------------

I'm tempted to handwave vaguely in the direction of m4. What with GNU
m4, and presumably a BSD family version or two, it'd be one less thing
to (re)invent.

----------------------------------------------------------------
> context [structure] can only be refered to by continuations,

Why not let GC handle the contexts too? Objects don't have to be
"first-class" or user-visible for GC to handle them, right?

----------------------------------------------------------------

> Yep. As said, we need variable sized register frames first [...]

That would be waiting until later.
--
Chip Salzenberg <ch...@pobox.com>

[ opt_count --> :opt_flag ]

> But seriously: I've been asking you to remove the counting feature

Sorry, I still don't see the "consistent" reason. The future dev might
include named arguments, which, for a general :opt_count, could imply
another scan through the arguments.

But reasoning on possible performance impacts within a feature that is
more heavy-weight anyway (hash lookups!) is not really a convincing
argument.

Having just an :opt_flag OTOH implies more arguments to be passed and
implies more used registers at the callee's code for more then 1
:optional. E.g.: PGE::Expr::new() has 3 :optional and 1 :opt_count. With
:opt_flag this would take 6 arguments to pass and 2 more used registers.
This will cause some slowdown.

> ... And the current user

Tcl is using :slurpy only currently, there are just 14 uses of
:opt_count in the whole parrot tree. It's not a big deal to change it,
*if* needed :-)

Ok. I'll put in colon aliases for all, the comma separator would be
optional for now.

> OK, but please add a FIXME comment.

Done.

Not PASM - PBC.

[ ref_count ]

Only PObjs (PMC, STRING) can be subject of GC. A context is a secondary
citizen, depending on the live-flag of continuations.

leo

Done (r9254). The canonical sequence for passing optional args now is:

.sub foo
.param <type> arg1 :optional
.param int has_arg1 :opt_flag
.param <type> arg2 :optional
.param int has_arg2 :opt_flag
...

leo