hi,
I've been studying the code in the fotw: debug.c and below are my comments,
if they're of any interest. Feel free to neglect, I'm kinda picky.
1.
while (*command && (isalnum((int) *command) || *command == ',' ||
*command == ']'))
I'm not 100% sure, but:
I've always been taught that this should not be written like this. The issue
here is that you ASSUME that the compiler does expression short-cutting
during evaluating the && expression. Suppose a c compiler does not implement
this, and evaluate both "*command" and the rest "(isalnum((int)*command ....
", and *command is NULL (well in this case I guess it will point to '\0'),
then things go wrong.
And I suppose we all know what ASSUME is an acronym for. :-)
I have to admit, I'm not 100% sure this issue is valid, but it sure seems
so. Relying on compilers' features is something I would not recommend.
This kind of code occurs in multiple spots in this file.
2. the define of "na". "na" stands for "next argument". Why not define it as
"next_argument", for sake of readability?
3. in the function 'parse_string', there's a variable called "string". I
don't think that's a good idea w.r.t. C++, which has the "string" datatype.
I think this var. is used to point to the first character in the string.
Maybe rename it to string_start.
Also, if I understand this code correctly, the double quoted string is
parsed out of the parameter "str". Maybe renaming this param to c_string
would be better?
4. I added some clarifying comments to the next function. Some may seem a
bit overkill, but try reading it in 4 weeks from now :-)
(and maybe rename "str" to "key"?) btw, the comments are more like why the
code is like this, and may help other people to improve it.
static const char*
parse_key(Interp *interp, const char* str, PMC** keyP)
{
*keyP = NULL; /* clear output parameter */
if (*str != '[') /* if there is no key, return a NULL key pointer */
return NULL;
str++; /* Skip [ */
if (*str == '"') {
STRING* string;
str = parse_string(interp, str, &string); /* parse_string can return
NULL. what happens then? */
*keyP = key_new_string(interp, string);
}
else if (isdigit((int) *str)) {
int value;
str = parse_int(str, &value);
*keyP = key_new_integer(interp, (INTVAL) value);
}
else { /* should this ever happen? The key was nor a string neither a
number; maybe this indicates an error? */
return NULL;
}
if (*str != ']') /* can this ever happen? why? */
return NULL;
return ++str;
}
5. in function PDB_get_command there's a variable called "line". It's marked
as a keyword, IIRC it's a directive of the C pre compiler?
If so, consider renaming it.
Ok, I see many more places to improve, but I'll leave it to this for now and
see if this first part of the file can be improved.
If desired, I'm willing to spend some more time on this, let me know.
Best regards,
kjs
