In message <[EMAIL PROTECTED]>
Mike Lambert <[EMAIL PROTECTED]> wrote:
> - assemble.pl:
> shouldn't the code :
> elsif ($_->[0] =~ /^([snpk])c$/) { # String/Num/PMC/Key constant
> include support for "kic" somewhere?
It doesn't need to as to_bytecode() turns [1] into an ic argument but
adds kic to the op name. Much the same thing is done for integer register
keys. So when _generate_bytecode() runs the argument type will just
appear to be a i or ic.
> 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?
Actually they correspond to the PARROT_ARG_XX constants uses for encoding
op arguments types. I should really add a perl version of those constants.
> - 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?
The reason I did it that way was that I wasn't sure whether a PMC that
was marked as constant could ever die before the end of the program, and
whether we might need to add and remove constant tables on the fly when
we load and unload bits of code.
Given that strings in the constant table are marked as constant I guess
that it should be safe to do the same for keys, so I have changed that.
> - 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?
Possibly. That was copied from the original. I'm not quite sure
what difference inout makes to the code that is generated?
> In your definition of the groups of set functions, can you change "Ax =
> Px[ KEY ]" to "Ax = Px[ INTKEY ]" where appropriate?
Done.
> - key.pmc
> the mark() function needs to return a value. Namely, the return value of
> key_mark.
Oops. That only went in yesterday... Now fixed.
> Overall, tho, the patch looks extemely complete. Tracing support,
> disassemble.pl support, debug.c support, etc. You even reduced macro
> usage. Rather impressive. :)
The tracing, disassembly etc was mostly done when I found I needed
it to try and find a problem in the other things I'd done ;-)
One other outstanding problem that I remembered last night is that
it is allocating memory for the key atom which is attached to the
cache.struct_val member on the PMC but which is never freed.
Allocating that small piece of memory as a buffer which can be GCed
seems like complete overkill, but short of marking the PMC as having
a private GC so it can cleanup I hadn't managed to come up with a
solution.
What I realised last night however is that there is enough space in
the private flags on the PMC for the type information and I can then
attach the data directly to the cache and do away with key atoms
completely... I shall get on with that now and post a new patch later.
Tom
--
Tom Hughes ([EMAIL PROTECTED])
http://www.compton.nu/
