Applied, and thanks for developing this code. However...
There was a whole bunch of problems with the reference
implementation, so I had to rework a lot of it when applying
to the core.
First off, the reference was full files not a patch. That's
really hard to work with given that the bytecode engine does
tend to evolve fairly quickly internally (just by virtue of
being a large piece of code!)Secondly, there were no tests.
If there had been, the many technical problems with the
patch would have shown up for you (it's behaviour was plain
wrong in a number of aspects!)
Thirdly, it inserted an instruction in the middle of the
bytecode list. Don't do that. I know it means you have to
do some hacking inside IllegalExprOperandType() but that's
better than breaking backward compatability for tbcload...
Fourthly, it had no documentation. Writing docs isn't hard
even in nroff; you can just copy the existing docs (that's
what I do normally!)
I attach the patch that I used. I don't know how easily it
will apply to anything, but it does fix all the faults I
know about so far.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I was convinced it was OK too, which is why it was an
unpleasant surprise when I hit problems when writing the
test suite! :^/ (I suppose this can stand as testimony to
why TIP implementations require a test suite in the first
place; its not easy to write covering tests, but it catches
ever so many silly mistakes that it is ever so worth it.)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Reference implementation
Logged In: YES
user_id=79902
Applied, and thanks for developing this code. However...
There was a whole bunch of problems with the reference
implementation, so I had to rework a lot of it when applying
to the core.
First off, the reference was full files not a patch. That's
really hard to work with given that the bytecode engine does
tend to evolve fairly quickly internally (just by virtue of
being a large piece of code!)Secondly, there were no tests.
If there had been, the many technical problems with the
patch would have shown up for you (it's behaviour was plain
wrong in a number of aspects!)
Thirdly, it inserted an instruction in the middle of the
bytecode list. Don't do that. I know it means you have to
do some hacking inside IllegalExprOperandType() but that's
better than breaking backward compatability for tbcload...
Fourthly, it had no documentation. Writing docs isn't hard
even in nroff; you can just copy the existing docs (that's
what I do normally!)
I attach the patch that I used. I don't know how easily it
will apply to anything, but it does fix all the faults I
know about so far.
Implementation unidiff patch
Logged In: YES
user_id=400048
Oh my, I do apologise for this - I had convinced myself that
all was okay. I will have to look into this more closely, so
as to learn from this!
Logged In: YES
user_id=79902
I was convinced it was OK too, which is why it was an
unpleasant surprise when I hit problems when writing the
test suite! :^/ (I suppose this can stand as testimony to
why TIP implementations require a test suite in the first
place; its not easy to write covering tests, but it catches
ever so many silly mistakes that it is ever so worth it.)