Thanks for putting this interesting and sophisticated bit of kit in the public domain. I have learned much from investigating it but have found some substantial bugs too.
The first is rather technical but affects debugging so takes precedence.
PreCompile::DebugDump (and the very structure of Instruction) expects that identifier tokens go in the mNameArg field of Instruction. However, PreCompiledExpr::Compile puts them into the mIntArg field by calling Emit(t, aExpression->GetLocation()) at line 148 instead of EmitName(...).
Setting this straight also requires changing the ILOAD, FLOAD and BLOAD cases in the switch statement of PrecompiledExpr::Evaluate (lines 273..5) so that they reference the mNameArg field. The expression mCode[++ip].mArgs.mIntArg needs to be modified to read mCode[++ip].mArgs.mNameArg.
The second bug is much worse in its effects on output involving any module with more than one argument. If I run a script for an identity function such as:-
var Alpha
axiom: Alpha(1, 2, 3)
p1: Alpha(int x, int y, int z) -> Alpha(x, y, z) ;
I find that odd iterations evaluate to Alpha(3, 2, 1) - the argument values have been reversed! Even though the even iterations give the expected result Alpha(1,2,3).
With genuine expressions the behavior is more complicated as its the out of place arguments that get evaluated.
The problem looks to me to stem from an assumption that the compiled arguments will be be in the natural order whereas bison's yyparse() function (for reasons best known to itself) presents them in reverse order. (something popped off a stack that was expected to be read as an array?)
My quick fix is simply to reverse the order that the arguments are parsed at LSystem::ParseModuleDef (line 253) by using a reverse iterator (or seeing as AST::ArgumentSpecList::ArgumentSpecs is an std::vector it can be done with old fashioned indices). This seems to work O.K. for simple cases at least.
Freeing LSystems from the grip of the turtle is a good move. I hope these fixes will help to reveal Lsie's true potential.
Tony Keele
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Merlin3D
Thanks for putting this interesting and sophisticated bit of kit in the public domain. I have learned much from investigating it but have found some substantial bugs too.
The first is rather technical but affects debugging so takes precedence.
PreCompile::DebugDump (and the very structure of Instruction) expects that identifier tokens go in the mNameArg field of Instruction. However, PreCompiledExpr::Compile puts them into the mIntArg field by calling Emit(t, aExpression->GetLocation()) at line 148 instead of EmitName(...).
Setting this straight also requires changing the ILOAD, FLOAD and BLOAD cases in the switch statement of PrecompiledExpr::Evaluate (lines 273..5) so that they reference the mNameArg field. The expression mCode[++ip].mArgs.mIntArg needs to be modified to read mCode[++ip].mArgs.mNameArg.
The second bug is much worse in its effects on output involving any module with more than one argument. If I run a script for an identity function such as:-
var Alpha
axiom: Alpha(1, 2, 3)
p1: Alpha(int x, int y, int z) -> Alpha(x, y, z) ;
I find that odd iterations evaluate to Alpha(3, 2, 1) - the argument values have been reversed! Even though the even iterations give the expected result Alpha(1,2,3).
With genuine expressions the behavior is more complicated as its the out of place arguments that get evaluated.
The problem looks to me to stem from an assumption that the compiled arguments will be be in the natural order whereas bison's yyparse() function (for reasons best known to itself) presents them in reverse order. (something popped off a stack that was expected to be read as an array?)
My quick fix is simply to reverse the order that the arguments are parsed at LSystem::ParseModuleDef (line 253) by using a reverse iterator (or seeing as AST::ArgumentSpecList::ArgumentSpecs is an std::vector it can be done with old fashioned indices). This seems to work O.K. for simple cases at least.
Freeing LSystems from the grip of the turtle is a good move. I hope these fixes will help to reveal Lsie's true potential.
Tony Keele