-
Notifications
You must be signed in to change notification settings - Fork 143
Fix incorrect behavior of 'error' function #303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1381,36 +1381,91 @@ void fatal(char *msg) | |||||
/* Reports an error and specifying a position */ | ||||||
void error(char *msg) | ||||||
{ | ||||||
/* Construct error source diagnostics, enabling precise identification of | ||||||
* syntax and logic issues within the code. | ||||||
*/ | ||||||
int offset, start_idx, i = 0; | ||||||
char diagnostic[512 /* MAX_LINE_LEN * 2 */]; | ||||||
/* Safety check for NULL message */ | ||||||
if (!msg) { | ||||||
printf("[Error]: Unknown error occurred\n"); | ||||||
abort(); | ||||||
} | ||||||
|
||||||
/* Safety check for SOURCE buffer */ | ||||||
if (!SOURCE || !SOURCE->elements || SOURCE->size < 0) { | ||||||
printf("[Error]: %s\nSource location unavailable (invalid source buffer)\n", msg); | ||||||
abort(); | ||||||
} | ||||||
Comment on lines
+1391
to
+1394
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not correctly handle OOB issue. |
||||||
|
||||||
/* Handle empty source case */ | ||||||
if (SOURCE->size == 0) { | ||||||
printf("[Error]: %s\nOccurs at start of file\n", msg); | ||||||
abort(); | ||||||
} | ||||||
|
||||||
/* Construct error source diagnostics */ | ||||||
int current_pos = SOURCE->size; | ||||||
int line_start, line_end; | ||||||
int i = 0; | ||||||
char diagnostic[512]; /* MAX_LINE_LEN * 2 */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I would prefer keeping all variable declarations at the top of the function. |
||||||
|
||||||
/* Ensure current_pos is within bounds */ | ||||||
if (current_pos >= SOURCE->size) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bounds check unconditionally shifts the error position one character left, causing off-by-one error in caret/column reporting. Prompt for AI agents
Suggested change
|
||||||
current_pos = SOURCE->size - 1; | ||||||
} | ||||||
|
||||||
for (offset = SOURCE->size; offset >= 0 && SOURCE->elements[offset] != '\n'; | ||||||
offset--) | ||||||
; | ||||||
/* Find the start of the current line (scan backwards to find '\n') */ | ||||||
line_start = current_pos; | ||||||
while (line_start > 0 && SOURCE->elements[line_start - 1] != '\n') { | ||||||
line_start--; | ||||||
} | ||||||
|
||||||
start_idx = offset + 1; | ||||||
/* Find the end of the current line (scan forwards to find '\n' or end) */ | ||||||
line_end = current_pos; | ||||||
while (line_end < SOURCE->size && SOURCE->elements[line_end] != '\n') { | ||||||
line_end++; | ||||||
} | ||||||
|
||||||
for (offset = 0; | ||||||
offset < MAX_SOURCE && SOURCE->elements[start_idx + offset] != '\n'; | ||||||
offset++) { | ||||||
diagnostic[i++] = SOURCE->elements[start_idx + offset]; | ||||||
/* Copy the current line to diagnostic buffer with bounds checking */ | ||||||
for (int pos = line_start; pos < line_end && i < (int)sizeof(diagnostic) - 50; pos++) { | ||||||
diagnostic[i++] = SOURCE->elements[pos]; | ||||||
} | ||||||
|
||||||
/* Add newline after the source line */ | ||||||
if (i < (int)sizeof(diagnostic) - 30) { | ||||||
diagnostic[i++] = '\n'; | ||||||
} | ||||||
diagnostic[i++] = '\n'; | ||||||
|
||||||
for (offset = start_idx; offset < SOURCE->size; offset++) { | ||||||
/* Add spaces to point to the error position */ | ||||||
int error_column = current_pos - line_start; | ||||||
for (int spaces = 0; spaces < error_column && i < (int)sizeof(diagnostic) - 20; spaces++) { | ||||||
diagnostic[i++] = ' '; | ||||||
} | ||||||
|
||||||
strcpy(diagnostic + i, "^ Error occurs here"); | ||||||
/* Add the error pointer with bounds checking */ | ||||||
const char *pointer_text = "^ Error occurs here"; | ||||||
int pointer_len = strlen(pointer_text); | ||||||
if (i + pointer_len < (int)sizeof(diagnostic)) { | ||||||
strcpy(diagnostic + i, pointer_text); | ||||||
i += pointer_len; | ||||||
} | ||||||
|
||||||
/* Null terminate */ | ||||||
if (i < (int)sizeof(diagnostic)) { | ||||||
diagnostic[i] = '\0'; | ||||||
} else { | ||||||
diagnostic[sizeof(diagnostic) - 1] = '\0'; | ||||||
} | ||||||
|
||||||
/* Calculate line number for better error reporting */ | ||||||
int line_number = 1; | ||||||
for (int pos = 0; pos < current_pos && pos < SOURCE->size; pos++) { | ||||||
if (SOURCE->elements[pos] == '\n') { | ||||||
line_number++; | ||||||
} | ||||||
} | ||||||
|
||||||
/* TODO: Implement line/column tracking for precise error location | ||||||
* reporting. Current implementation only shows source position offset. | ||||||
*/ | ||||||
printf("[Error]: %s\nOccurs at source location %d.\n%s\n", msg, | ||||||
SOURCE->size, diagnostic); | ||||||
/* Output the error with improved formatting */ | ||||||
printf("[Error]: %s\n", msg); | ||||||
printf("At line %d, column %d (source position %d):\n", | ||||||
line_number, error_column + 1, current_pos); | ||||||
printf("%s\n", diagnostic); | ||||||
Comment on lines
+1464
to
+1468
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not resolve the goal of TODO, as the line number and column number could only be meaningful when file name persists. With only line number and column number it's quite meaningless. |
||||||
abort(); | ||||||
} | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of adding a NULL check here?
AFAICS there is no case where a NULL pointer is passed in, so this doesn't address any real security issue.
Also, I don't see a strong reason to change the API to allow NULL pointers as valid input.