On 5/23/07, Klaas-Jan Stol <[EMAIL PROTECTED]> wrote:
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
Attached a patch that adds some more comments that explain the code as far as i can see. There are some parts in the file that I don't get a grip on. hth, kjs
more_debug_comments.patch
Description: Binary data
