> Attached is my first cut of a patch to address the keyed access issues.
First, thanks for spending the time to implement and clean up the keyed
code. Hopefully this'll clean the floor so that when this list has key
discussions, we'll all be arguing about the same thing. :)
> This patch doesn't do everything, but it does bring things more or less
> in line with Dan's recent specification I hope. I'm sure there are also
> problems with it so if we can get some eyeballs on it before I commit it
> that would be good.
Here are the things I noticed when going through your patch.
- assemble.pl:
shouldn't the code :
elsif ($_->[0] =~ /^([snpk])c$/) { # String/Num/PMC/Key constant
include support for "kic" somewhere?
the magic numbers in _key_constant, I'm assuming they are supposed to map
to the constants in key.h ? Perhaps a note mentioning that correspondance
would be useful. Also, it seems the number usage is broken. You use
1,1,1,2,4,7. Shouldn't it be 1,1,1,2,3,5? And shouldn't s/inps/ be
s/insp/? Or maybe the constants in key.h need rearranging?
- dod.c:
Near the comment, "Mark the key constants as live". Constants shouldn't
need to be marked live, as constants are prevented from being GC'ed, if
PMC_constant_FLAG is set. At least, in theory. Did it not work for you?
- core.ops
Looking at the set functions, shouldn't the "Px[ KEY ] = Bx"
set of functions have $1 defined as inout instead of out in most
circumstances?
In your definition of the groups of set functions, can you change "Ax =
Px[ KEY ]" to "Ax = Px[ INTKEY ]" where appropriate?
- key.pmc
the mark() function needs to return a value. Namely, the return value of
key_mark.
- random
Your use of registers for key atom values is interesting, and I think it
will create problems. It's not a problem with your patch as much a
problem with an aspect of the key design, I think.
The plan is to allow parrot functions to implement vtable methods in
parrot. If I have a key [I0,I1], and pass it to this vtable method, it
could be passed to a function implemented in parrot, with all of parrot's
calling conventions. This means that by the time it gets to the person
implementing the key, it's extremely possible that the registers have been
overwritten.
I'm not sure how to resolve this one. Alternatives are:
a) don't allow register references in keys. Instead, force people to use
the key modification ops to reset the key to the correct values each time
they want to use it.
b) handle auto-generated .ops files, such that if they receive a KEY as a
parameter, it calls key_fixup_registers, which grabs the current values of
the registers and sticks them into the key structure. This could cause
problems with constant keys, so you might need to create a key copy.
c) any other ideas? Or should we mark this as a 'known limitation' ?
Overall, tho, the patch looks extemely complete. Tracing support,
disassemble.pl support, debug.c support, etc. You even reduced macro
usage. Rather impressive. :)
Thanks,
Mike Lambert
