On Thu, 2010-09-23 at 20:21 +0200, Karel Klic wrote:
/**
- A location in the backtrace file with an attached message.
- It's used for error reporting: the line and the column points to
- the place where a parser error occurred, and the message explains
- what the parser expected and didn't find on that place.
*/ struct btp_location { /** Starts from 1. */ int line; /** Starts from 0. */ int column; /** * Error message related to the line and column. Do not release * the memory this pointer points to. */ const char *message; };
IIUC you are trying to get accurate error reporting, and for that you are recording the position while you consume input, and record message and return with it on error. End user uses this machinery like this:
btp_thread_parse(&input_char_ptr, ..., ptr_to_location_struct); if (ERROR) { printf("%d:%d: %s, run for your lives", location.line, location.column, location.message); }
The goal is sound, but the price you pay to achieve it seems to be high in terms of code simplicity and readability.
IMHO the added complexity is worth the benefits. The location struct is not really complex, and the column/row counting code even helped me to discover some syntax corner cases which were previously not handled.
I would like to make it simpler. How about (1) printing (logging) error message on the spot instead of propagating it back to caller? (2) passing two char* pointers down to callees:
btp_thread_parse(&input_char_ptr, ...): start = input_char_ptr; while (...) -> btp_thread_parse(start,&input_char_ptr, ....) -> btp_frame_parse(start,&input_char_ptr, ....) if (ERROR DETECTED) btp_log_error(start, *input_char_ptr, "Titanic hit the iceberg") -> calculates line:col by walking from start to cur = *input_char_ptr and counting \n and other chars, then log("%d:%d: %s", line, col, msg)
This way, not only struct btp_location is not needed anymore, but also a lot of "location->column += chars" statements can be dropped, along with calls to btp_location_add* and btp_location_eat*. They don't need to be replaced, they simply aren't needed at all.
I think the location code provides a good feature and it should not cause problems. It seems we have different opinion about the code simplicity/features balance.
I am not proposing to remove it. I am saying that it can be implemented simpler.
But I think we can postpone this simplification to a later time.
Other things I noticed:
char * btp_file_to_string(const char *filename) { /* Open input file, and parse it. */ FILE *fp = fopen(filename, "r"); if (!fp) { fprintf(stderr, "Unable to open '%s'.\n", filename); return NULL; }
fseek(fp, 0, SEEK_END); size_t size = ftell(fp); fseek(fp, 0, SEEK_SET); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This will fail on large files. Of course, using fseeko/ftello, this can be fixed, but see below...lseek() is better! :)
static const size_t FILE_SIZE_LIMIT = 20000000; /* ~ 20 MB */ if (size > FILE_SIZE_LIMIT) { fprintf(stderr, "Input file too big (%zd). Maximum size is %zd.\n", size, FILE_SIZE_LIMIT); fclose(fp); return NULL; }
char *contents = btp_malloc(size + 1);
if (1 != fread(contents, size, 1, fp)) { fprintf(stderr, "Unable to read from '%s'.\n", filename); fclose(fp); free(contents); return false; ^^^^^^^^^^^^^ ?! did you mean "return NULL"? }
fclose(fp); contents[size] = '\0'; return contents; }
Here, since you need neither buffering nor formatting, you can simply use direct open/lseek/read/close interface, not fopen/fseek/fread/fclose machinery. In C, it will be a bit simpler; and under surface, you will avoid glibc's mallocing and buffering altogether.
int btp_skip_hexadecimal_number(char **input) { char *local_input = *input; if (!btp_skip_char(&local_input, '0')) return 0; if (!btp_skip_char(&local_input, 'x')) return 0; int count = 2; count += btp_skip_char_span(&local_input, "abcdef0123456789"); if (2 == count) /* btp_skip_char_span returned 0 */ return 0; *input = local_input; return count; }
Will it (does it need to) handle uppercase hex digits correctly?
Several instances of code like this:
while (frame) { /* Remove frames which are not a cause of the crash. */ bool removable = btp_frame_calls_func(frame, "__kernel_vsyscall"); if (removable) { struct btp_frame *next_frame = frame->next; btp_thread_remove_frame(thread, frame); frame = next_frame; continue; }
frame = frame->next; }
can be a bit simpler:
while (frame) { struct btp_frame *next_frame = frame->next; /* Remove frames which are not a cause of the crash. */ bool removable = btp_frame_calls_func(frame, "__kernel_vsyscall"); if (removable) { btp_thread_remove_frame(thread, frame); }
frame = next_frame; }
Apart from these small notes, the code looks good to be included in git. Thanks!