On 5/23/07, chromatic <[EMAIL PROTECTED]> wrote:
This file implements most of the Parrot debugger. The interpreter struct
has
a slot called pdb that contains a PDB_t (parrot/debug.h).
This file is somewhat messy. It has some string manipulation functions
(nextarg(), skip_ws(), parse_int(), parse_string()) that should probably
go
elsewhere.
There are also some places that seem somewhat careless about memory
allocation
and freeing. For example, where in this file does interp->pdb get
initialized? (Answer: in src/embed.c - Parrot_disassemble()).
Where does it get freed? (Answer: nowhere that I can tell.)
The freeing *could* go in Parrot_really_destroy() in src/inter_create.c
(did
you catch the contradiction in names there?), but I'm starting to think
that
each file that represents the entry point into a system should have two
functions, one that initializes the system and its necessary data
structures
and another that finalizes and frees things.
I don't know if we have any good tests for the debugger; this is something
we
ought to consider if we're going to move code around. Sadly, I don't know
any easy way to test things apart from opening a Parrot process and
feeding
data in and out. Making the debugger scriptable from PIR is a bigger
project
than I'm comfortable suggesting until it gets more tests.
Some of the other memory-related functions have a little bit too much
magic.
For example, PDB_free_file() takes the file to free out of the current
debugger. It does the right thing to free files, but there appear to be
cases where it's useful to free a file that's not the debugger's current
file, so this function is inappropriately general.
Other functions have odd names -- PDB_hasInstructions() (no underscore?),
PDB_print() (should be PDB_print_registers()).
The code is fairly decent. Most of the issues here relate to
organization.
-- c
There are some magic numbers, like 255, and some other very unclear code
snippets like:
for (i = 0; *command && isalpha((int) *command); command++, i++)
c += (tolower((int) *command) + (i + 1)) * ((i + 1) * 255);
This needs some comments. If anybody knows what's going on there, please
enlighten me and fellow readers :-)
regards,
kjs
