Update expr.c #91
Update expr.c #91
Conversation
ohkimur
left a comment
ohkimur
left a comment
There was a problem hiding this comment.
Thanks for the PR! The change from pre-increment/decrement to post-increment/decrement looks correct and fixes the off‑by‑one behavior:
- In
push,stack[stack_pointer++] = element;is the right choice ifstack_pointertracks the next free slot. - In
pop,return stack[--stack_pointer];correctly moves back to the last pushed element before reading.
A couple of suggestions to strengthen this and future contributions:
- Add brief comments documenting the stack invariant: “
stack_pointerpoints to the next free slot.” - Include small tests (or a demo
main) for: push/pop single item, push until full, pop until empty, and verify strict LIFO. - Consider returning a status from
push/pop(e.g., int/bool with out‑param) instead of printing errors; returning0frompopis ambiguous if0.0is a valid value. - Use
size_tforstack_pointerand comparisons withSTACK_SIZE. - Optionally guard against overflow/underflow with asserts in debug builds.
Overall, nice, focused fix with clear correctness improvement. If you’re up for it, similar sanity checks and invariants across other stack/queue exercises would be super valuable.
Thank you for the suggestions, I would be happy to contribute more (if my time allows)! |
|
@JebediahS Hello, are you still interested to contribute on this PR, or shall I close it? |
Sorry for the delay, still interested! |
ohkimur
left a comment
ohkimur
left a comment
There was a problem hiding this comment.
@JebediahS Hello, are you still interested to contribute on this PR, or shall I close it?
Sorry for the delay, still interested!
Great! Whenever you want to merge this PR, just make sure you checkout the new CONTRIBUTIONS.md file, as this a new addition to the repo so it makes the code more consistent and a bit more modern.
ohkimur
left a comment
ohkimur
left a comment
There was a problem hiding this comment.
@JebediahS Also, you'll need to merge the new changes from main first in your local branch to solve the merge conflicts.
To not mess up your style: In which form do you expect the tests to be set up? Should there be a test-folder |
|
@coderabbitai Can you help us here? What would be the best way to create tests in this repo, considering it's K&R C Programming Book solutions and it should be accessible to newcomers? Offer us some useful approaches here. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4580 🏁 Script executed: Length of output: 7328 @ohkimur Great question! Based on the repository structure, I can see this is an educational repo focused on K&R exercises. I found one existing test file ( Approach 1: Inline Test Main (Recommended for newcomers)Add a test // At the end of expr.c, after the existing main()
#ifdef TEST_MODE
int main(void)
{
printf("Testing expr stack operations...\n");
// Test 1: Push and pop single element
printf("Test 1: Push 5.0, pop should return 5.0\n");
push(5.0);
float result = pop();
printf(" Result: %.1f %s\n", result, (result == 5.0) ? "PASS" : "FAIL");
// Test 2: LIFO order
printf("Test 2: Push 1,2,3, pop should return 3,2,1\n");
push(1.0);
push(2.0);
push(3.0);
float r3 = pop();
float r2 = pop();
float r1 = pop();
printf(" Result: %.1f,%.1f,%.1f %s\n", r3, r2, r1,
(r3 == 3.0 && r2 == 2.0 && r1 == 1.0) ? "PASS" : "FAIL");
// Test 3: Fill stack
printf("Test 3: Fill stack to STACK_SIZE\n");
for (int i = 0; i < STACK_SIZE; i++) {
push((float)i);
}
printf(" Stack pointer: %d (should be %d) %s\n", stack_pointer, STACK_SIZE,
(stack_pointer == STACK_SIZE) ? "PASS" : "FAIL");
// Test 4: Empty stack
printf("Test 4: Empty entire stack\n");
for (int i = 0; i < STACK_SIZE; i++) {
pop();
}
printf(" Stack pointer: %d (should be 0) %s\n", stack_pointer,
(stack_pointer == 0) ? "PASS" : "FAIL");
return 0;
}
#endifCompile and run: gcc -DTEST_MODE -o expr_test chapter_5/exercise_5_10/expr.c -lm
./expr_testPros: No extra files, easy for beginners, tests live with code Approach 2: Separate Test File (Consistent with existing pattern)Create #include <stdio.h>
#include <stdlib.h>
// Declare the external functions and variables
extern unsigned char stack_pointer;
extern float stack[];
extern void push(float element);
extern float pop(void);
void test_push_pop_single(void) {
printf("Test: Push and pop single element... ");
push(42.5);
float result = pop();
printf("%s\n", (result == 42.5 && stack_pointer == 0) ? "PASS" : "FAIL");
}
void test_lifo_order(void) {
printf("Test: LIFO order... ");
push(1.0);
push(2.0);
push(3.0);
int pass = (pop() == 3.0 && pop() == 2.0 && pop() == 1.0);
printf("%s\n", pass ? "PASS" : "FAIL");
}
void test_fill_stack(void) {
printf("Test: Fill stack to capacity... ");
for (int i = 0; i < 15; i++) {
push((float)i);
}
printf("%s\n", (stack_pointer == 15) ? "PASS" : "FAIL");
// Clear for next test
for (int i = 0; i < 15; i++) pop();
}
int main(void) {
printf("=== Stack Implementation Tests ===\n");
test_push_pop_single();
test_lifo_order();
test_fill_stack();
printf("=== Tests Complete ===\n");
return 0;
}You'll need to modify Compile and run: gcc -o expr_test chapter_5/exercise_5_10/expr.c chapter_5/exercise_5_10/expr_test.c -lm
./expr_testPros: Follows existing pattern, separates concerns Approach 3: Shell Script Test RunnerCreate #!/bin/bash
echo "=== Testing expr calculator ==="
# Build the program
gcc -o expr chapter_5/exercise_5_10/expr.c -lm
# Test 1: Simple addition
result=$(./expr 2 3 +)
expected="result: 5.000"
if [[ "$result" == "$expected" ]]; then
echo "✓ Test 1 PASS: 2 + 3 = 5"
else
echo "✗ Test 1 FAIL: Expected '$expected', got '$result'"
fi
# Test 2: Subtraction
result=$(./expr 10 3 -)
expected="result: 7.000"
if [[ "$result" == "$expected" ]]; then
echo "✓ Test 2 PASS: 10 - 3 = 7"
else
echo "✗ Test 2 FAIL: Expected '$expected', got '$result'"
fi
# Test 3: Complex expression
result=$(./expr 5 1 2 + 4 \* + 3 -)
expected="result: 14.000"
if [[ "$result" == "$expected" ]]; then
echo "✓ Test 3 PASS: Complex RPN expression"
else
echo "✗ Test 3 FAIL: Expected '$expected', got '$result'"
fi
# Test 4: Division by zero
result=$(./expr 5 0 / 2>&1)
if [[ "$result" == *"division by zero"* ]]; then
echo "✓ Test 4 PASS: Division by zero detected"
else
echo "✗ Test 4 FAIL: Should detect division by zero"
fi
# Cleanup
rm -f expr
echo "=== Tests Complete ==="Run: Pros: Tests end-to-end behavior, no code changes needed, easy to understand My Recommendation for This RepositoryFor a K&R solutions repo targeting newcomers, I'd suggest a hybrid approach:
This maintains simplicity while providing practical testing examples that teach good habits. The shell script approach is especially good here because:
Would you like me to prepare a complete test suite example for the |
WalkthroughAdds operator counting and missing-operator detection, adjusts stack push/pop to 0-based semantics, and introduces a new Bash test script exercising arithmetic, RPN expression, division-by-zero, and missing-operator cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)chapter_5/exercise_5_10/expr.c (3)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
chapter_4/exercise_4_14/swap.c-3-9 (1)
3-9: Remove trailing semicolon and wrap macro in do-while(0) for safety.The macro has two issues that make it unsafe in certain contexts:
The trailing semicolon on Line 9 causes
swap(int, x, y);to expand to{ ... };;, creating a double semicolon. While harmless here, it's non-idiomatic.More importantly, the macro lacks a
do-while(0)wrapper, which is the standard idiom for multi-statement macros. Without it, the macro can break in control-flow contexts (e.g., if-else chains without braces).Apply this diff to fix both issues:
-#define swap(t, x, y) \ - { \ - t temp; \ - temp = x; \ - x = y; \ - y = temp; \ - }; +#define swap(t, x, y) \ + do { \ + t temp; \ + temp = x; \ + x = y; \ + y = temp; \ + } while (0)This ensures the macro behaves like a single statement and can be safely used with a semicolon at the call site.
chapter_5/exercise_5_13/tail.c-55-62 (1)
55-62: Fixstrlenin loop and handle empty string edge case.The function has two issues:
- Calling
strlenin the loop condition results in O(n²) complexity.- Empty string returns 1 (valid), which allows "-" as a valid argument.
Apply this diff:
int is_str_uint(char *str) { - for (size_t i = 0; i < strlen(str); ++i) { + if (*str == '\0') { + return 0; + } + for (size_t i = 0; str[i] != '\0'; ++i) { if (!isdigit(str[i])) { return 0; } } return 1; }chapter_5/exercise_5_13/tail.c-33-37 (1)
33-37: Usestrtoulinstead ofatoifor parsing unsigned values.
atoireturnsintand cannot properly validate or parse values that exceedINT_MAXor handle overflow. Sincenr_of_lines_to_printissize_t, usestrtoulfor proper unsigned parsing with error detection.Apply this diff:
- size_t nr_of_lines_to_print = DEFAULT_NR_OF_LINES_TO_PRINT; - - if (argc == 2) { - nr_of_lines_to_print = atoi(argv[argc - 1] + 1); - } + size_t nr_of_lines_to_print = DEFAULT_NR_OF_LINES_TO_PRINT; + + if (argc == 2) { + char *endptr; + unsigned long val = strtoul(argv[argc - 1] + 1, &endptr, 10); + if (*endptr != '\0' || val > SIZE_MAX) { + puts("Error: invalid number.\n"); + return EXIT_FAILURE; + } + nr_of_lines_to_print = (size_t)val; + }chapter_1/exercise_1_18/trailing_blanks.c-38-46 (1)
38-46: Missing bounds check can cause buffer underrun.The loop
for (i = length - 2; line[i] == ' ' || line[i] == '\t'; --i)lacks a lower bound check. If a line consists entirely of whitespace (e.g.," \n"),iwill decrement past 0, causing undefined behavior when accessingline[i].void remove_trailing_blanks(char line[], int length) { int i; - for (i = length - 2; line[i] == ' ' || line[i] == '\t'; --i) + for (i = length - 2; i >= 0 && (line[i] == ' ' || line[i] == '\t'); --i) ; line[i + 1] = '\n'; line[i + 2] = '\0'; }chapter_1/exercise_1_11/word_count_test.c-16-17 (1)
16-17: Useintforgetchar()return value to correctly detect EOF.
getchar()returnsintto distinguish EOF (-1) from valid characters. Usingcharcan cause EOF detection to fail (on unsigned char) or misinterpret valid 0xFF bytes as EOF (on signed char).- char c; - while ((c = getchar()) != EOF) { + int c; + while ((c = getchar()) != EOF) {chapter_5/exercise_5_05/strncat.c-19-28 (1)
19-28: Off-by-one error and missing null termination.The loop condition
(*s++ = *t++) != '\0' && n--has two issues:
Off-by-one: The post-decrement
n--is evaluated after the character is copied due to short-circuit evaluation. Whenn=5, this copies 6 characters beforen--returns 0 (false).Missing null terminator: If
tis longer thanncharacters, the result is not null-terminated.Apply this diff to fix both issues:
void strcat_ptr(char *s, char *t, size_t n) { // Find the end of s size_t s_length = strlen(s); // Move the s pointer to the end of the s string. s += s_length; // copy t to the end of s - while ((*s++ = *t++) != '\0' && n--) - ; + while (n-- > 0 && (*s++ = *t++) != '\0') + ; + if (*(s - 1) != '\0') + *s = '\0'; }Alternatively, a cleaner approach:
void strcat_ptr(char *s, char *t, size_t n) { s += strlen(s); while (n-- > 0 && *t != '\0') *s++ = *t++; *s = '\0'; }chapter_5/exercise_5_14/sort.c-42-49 (1)
42-49: Potential underflow when input is empty.If
read_linesreturns 0 (empty input), the expressionnr_of_lines - 1wraps toSIZE_MAXsincesize_tis unsigned. This passes an enormous range toquick_sort, causing undefined behavior.Add a check for zero lines before calling
quick_sort:if ((nr_of_lines = read_lines(line_ptr, MAX_NR_OF_LINES)) != (size_t)-1) { + if (nr_of_lines > 0) { quick_sort((void **)line_ptr, 0, nr_of_lines - 1, (int (*)(void *, void *))comp); + } write_lines(line_ptr, nr_of_lines); } else {chapter_5/exercise_5_19/undcl.c-95-103 (1)
95-103: Off-by-one: null terminator can write past buffer if name fillsmax_len.If exactly
max_lenalphanumeric characters are read,ibecomesmax_lenanddest[i] = '\0'writes one byte past the buffer. The loop should usei < max_len - 1to reserve space for the null terminator.void get_name(char *dest, const size_t max_len) { int c; size_t i = 0; - while ((isalnum(c = getc(stdin)) || c == '_') && i < max_len) { + while ((isalnum(c = getc(stdin)) || c == '_') && i < max_len - 1) { dest[i++] = c; } dest[i] = '\0'; ungetc(c, stdin); }chapter_4/exercise_4_05/math.c-150-154 (1)
150-154: Sameclear()bug: signed integer underflow.Identical issue to
chapter_4/exercise_4_06/variables.c: whenspis 0, thedo-whilestill executes and decrementsspto -1, causing undefined behavior on subsequent operations.void clear(void) { - do { - stack[sp] = 0.0; - } while (sp--); + sp = 0; }chapter_4/exercise_4_06/variables.c-105-109 (1)
105-109: Missing bounds check forvarindexmay cause buffer overflow.
var_buffhasVARNUM(26) slots, butvarindexis incremented without validation. If more than 26 variables are assigned, this writes past the array bounds.case VARSET: + if (varindex >= VARNUM) { + printf("Error: variable limit reached.\n"); + break; + } var_buff[varindex++] = pop(); printf("variable %c: %.3f\n", 'a' + varindex - 1, var_buff[varindex - 1]); break;chapter_5/exercise_5_19/undcl.c-82-90 (1)
82-90: Bug: block comment parsing doesn't correctly find*/.The loop at line 83 stops at the first
'*'character, not at*/. For a comment like/* a * b */, it would stop at the first*(aftera), then read the space, which is not/, and the function returns with incorrect state.The logic should look for
*/as a pair:} else if (c == '*') { - while ((c = getc(stdin)) != '*' && c != EOF) - ; - c = getc(stdin); - if (c == '/') { - ungetc('\n', stdin); - return; + int prev = 0; + while ((c = getc(stdin)) != EOF) { + if (prev == '*' && c == '/') { + ungetc('\n', stdin); + return; + } + prev = c; } }chapter_4/exercise_4_06/variables.c-186-190 (1)
186-190: Undefined behavior:clear()causes signed integer underflow and out-of-bounds access.When
spis 0 (empty stack), thedo-whileloop still executes once, accessingstack[0](harmless), but thensp--decrementsspto -1. The conditionsp--evaluates the old value (0), which is falsy, so the loop exits—butspis now -1. Subsequentpush/popcalls will misbehave. Additionally, ifsp > 0, the loop clearsstack[sp]which is one past the last valid element.Consider using a simple assignment instead:
void clear(void) { - do { - stack[sp] = 0.0; - } while (sp--); + sp = 0; }Zeroing stack contents is unnecessary since
spcontrols valid entries.chapter_5/exercise_5_20/dcl.c-112-120 (1)
112-120: Block comment parsing exits prematurely on any*.Same issue as in
count_c_keywords.c: the while loop exits on any*, not just the closing*/. A comment like/* ptr * 2 */would be incorrectly parsed.See the fix suggested for
chapter_6/exercise_6_01/count_c_keywords.cwhich uses a two-character lookahead to properly detect*/.chapter_4/exercise_4_10/calculator.c-118-120 (1)
118-120: Missing validation for variable index inVARGET.
var - 'a'is used as an index without validation. Ifvaris uppercase or outside 'a'-'z', the index could be out of bounds (e.g.,'A' - 'a' = -32).case VARGET: + if (var < 'a' || var > 'z') { + printf("Error: invalid variable '%c'.\n", var); + break; + } push(var_buff[var - 'a']); break;chapter_4/exercise_4_10/calculator.c-112-116 (1)
112-116: Missing bounds check onvarindexcan cause buffer overflow.
varindexis incremented without checking againstVARNUM(26). If the user executes the_command more than 26 times,var_buff[varindex++]writes past the array bounds.case VARSET: + if (varindex >= VARNUM) { + printf("Error: variable storage full.\n"); + break; + } var_buff[varindex++] = pop(); printf("variable %c: %.3f\n", 'a' + varindex - 1, var_buff[varindex - 1]); break;chapter_6/exercise_6_01/count_c_keywords.c-70-78 (1)
70-78: Block comment parsing exits prematurely on any*.The while loop at Line 71 exits when it encounters any
*, not just the closing*/. For a comment like/* hello * world */, it stops at the first*after "hello", then checks if the next character is/. Since it's a space, the function returns early, leaving* world */in the input stream.} else if (c == '*') { - while ((c = getc(stdin)) != '*' && c != EOF) - ; - c = getc(stdin); - if (c == '/') { - ungetc('\n', stdin); - return; + int prev = 0; + while ((c = getc(stdin)) != EOF) { + if (prev == '*' && c == '/') { + ungetc('\n', stdin); + return; + } + prev = c; } }chapter_6/exercise_6_06/define.c-103-122 (1)
103-122: Memory leak ifstr_dup(definition)fails after node allocation.When
lookup(name)returns NULL, a new node is allocated andnode_p->nameis set. Ifstr_dup(definition)on line 117 subsequently fails and returns NULL, the function returns NULL but the allocated node (with its duplicated name) remains in the hash table, causing a memory leak and a corrupted entry.Consider removing the node from the hash table before returning NULL:
if ((node_p->definition = str_dup(definition)) == NULL) { + // Clean up: remove partially initialized node from hash table + if (prev_lookup_result == NULL) { + // Node was just inserted; need to remove it + size_t hv = hash(name); + hash_table[hv] = node_p->next; + free(node_p->name); + free(node_p); + } return NULL; }Alternatively, defer insertion into the hash table until both allocations succeed.
Committable suggestion skipped: line range outside the PR's diff.
chapter_6/exercise_6_03/cross_referencer.c-155-166 (1)
155-166: Missing NULL check after malloc inadd_to_list.If
mallocfails on line 158, the subsequent dereferences on lines 159-160 will cause a null pointer dereference.struct list_node *add_to_list(struct list_node *list_node_p, size_t line_number) { if (list_node_p == NULL) { list_node_p = (struct list_node *)malloc(sizeof(struct list_node)); + if (list_node_p == NULL) { + return NULL; + } list_node_p->line_number = line_number; list_node_p->next = NULL; } else { list_node_p->next = add_to_list(list_node_p->next, line_number); } return list_node_p; }chapter_5/exercise_5_11/detab.c-26-31 (1)
26-31: Potential division by zero when tab_stop is 0.If a user passes "0" as a tab stop argument,
is_str_uintwill accept it (since '0' is a digit), butatoi(argv[arg_pos++])on line 27 will settab_stop = 0. This causes undefined behavior on line 33 due toline_pos % tab_stop.Consider rejecting zero or non-positive tab stops in the validation function:
int is_str_uint(char *str) { + if (str[0] == '\0') { + return 0; + } for (size_t i = 0; i < strlen(str); ++i) { if (!isdigit(str[i])) { return 0; } } - return 1; + return atoi(str) > 0; }Committable suggestion skipped: line range outside the PR's diff.
chapter_6/exercise_6_05/undef.c-79-98 (1)
79-98: Memory leak and corrupted hash table on allocation failure ininstall.If
str_dup(definition)fails on line 93 for a new node, the function returnsNULLbut leaves a partially initialized node in the hash table (inserted at lines 87-88). This node hasnode_p->definitionuninitialized, which will cause issues on subsequent lookups or when freeing.Similarly, if
str_dup(name)fails on line 83,node_pis leaked.Consider restructuring to avoid inserting until all allocations succeed:
struct list_node *install(char *name, char *definition) { struct list_node *node_p; if ((node_p = lookup(name)) == NULL) { node_p = (struct list_node *)malloc(sizeof(*node_p)); - if (node_p == NULL || (node_p->name = str_dup(name)) == NULL) { + if (node_p == NULL) { + return NULL; + } + node_p->name = str_dup(name); + node_p->definition = str_dup(definition); + if (node_p->name == NULL || node_p->definition == NULL) { + free(node_p->name); + free(node_p->definition); + free(node_p); return NULL; } size_t hash_value = hash(name); node_p->next = hash_table[hash_value]; hash_table[hash_value] = node_p; - } else { - free(node_p->definition); + return node_p; } - if ((node_p->definition = str_dup(definition)) == NULL) { + // Update existing node + char *new_def = str_dup(definition); + if (new_def == NULL) { return NULL; } - + free(node_p->definition); + node_p->definition = new_def; return node_p; }
🟡 Minor comments (26)
chapter_1/exercise_1_13/histogram.c-47-48 (1)
47-48: Add bounds check to prevent potential buffer overflow.If
histogram_lengthreachesBUFFER - 1(99), the assignment on line 48 accesseshistogram[BUFFER], which is out of bounds. While this requires processing a word of 100 characters, it's still a correctness issue.Apply this diff to add a bounds check:
// Add in the histogram array a end of useful data char - histogram[histogram_length + 1] = '$'; + if (histogram_length + 1 < BUFFER) { + histogram[histogram_length + 1] = '$'; + }Alternatively, ensure that
word_count_indexnever exceedsBUFFER - 1during the counting phase by adding a check before incrementing on line 43.chapter_5/exercise_5_04/strend.c-7-15 (1)
7-15: Minor: User-facing text contains typos.The output strings contain repeated spelling errors:
- Line 13, 15, 19, 21: "orrurs" should be "occurs"
- Line 13, 19: "orrur" should be "occur"
Apply this diff to fix the typos:
- puts("The string t1 orrurs at the end of the string s."); + puts("The string t1 occurs at the end of the string s."); else - puts("The string t1 doesn't orrur at the end of the string s."); + puts("The string t1 doesn't occur at the end of the string s."); // Test if the string t2 occurs at the end of string s. if (strend(s, t2)) - puts("The string t2 orrurs at the end of the string s."); + puts("The string t2 occurs at the end of the string s."); else - puts("The string t2 doesn't orrur at the end of the string s."); + puts("The string t2 doesn't occur at the end of the string s.");Also applies to: 18-21
chapter_5/exercise_5_10/expr.c-97-101 (1)
97-101: Handle the empty argument case.When no arguments are provided (
argc == 1), the condition on line 97 is false, and line 100 executespop()on an empty stack. This produces confusing output: the error message "Error: the stack is empty." followed by "result: 0.000".Additionally, line 98 is missing a newline character, and the final result print will execute even when an operator is missing.
Apply this diff to handle edge cases:
if (op_count == 0 && argc > 2) { - printf("Error: Operator is missing!"); + printf("Error: Operator is missing!\n"); + return EXIT_FAILURE; + } else if (stack_pointer == 0) { + printf("Error: No arguments provided.\n"); + return EXIT_FAILURE; + } else if (stack_pointer > 1) { + printf("Error: Too many operands, missing operators.\n"); + return EXIT_FAILURE; } else { printf("result: %.3f", pop()); }chapter_5/exercise_5_10/expr.c-26-26 (1)
26-26: Remove overly restrictive stack size check.The condition
stack_pointer < STACK_SIZEis unnecessary here. Binary operations pop 2 elements and push 1 (net reduction of 1), so they can safely execute even whenstack_pointer == STACK_SIZE(full stack). The current check incorrectly prevents valid operations when the stack is full.Apply this diff:
- if (stack_pointer >= 2 && stack_pointer < STACK_SIZE) { + if (stack_pointer >= 2) { float number2 = pop();chapter_1/exercise_1_11/word_count_test.c-7-9 (1)
7-9: Useintinstead ofcharfor counters.Declaring
nl,nw, andncascharlimits their range to ~127 (signed) or 255 (unsigned), causing overflow for any moderately sized input. These should beintto handle realistic counts.- char nl; - char nw; - char nc; + int nl; + int nw; + int nc;chapter_2/exercise_2_03/htoi.c-53-60 (1)
53-60: Error handling doesn't halt conversion.When an invalid hex character is detected, an error message is printed but the loop continues, and the invalid character still contributes to
resultvia thetempvalue from the previous checks. Consider returning early or usingbreak.if ((hex[i] >= '0' && hex[i] <= '9') || (hex[i] >= 'a' && hex[i] <= 'f') || (hex[i] >= 'A' && hex[i] <= 'F')) { result += temp * (int)pow(16, len - i - 1); } else { printf("Error: Not a valid hex value.\n Try this format: 0xHHHH, " "where H is a hex digit.\n"); + return -1; // or another sentinel value }chapter_2/exercise_2_03/htoi.c-38-41 (1)
38-41: Prefix check runs on every iteration.The
0x/0Xprefix check is inside the while loop, so it could match mid-string (e.g., input"120xAB"would skip to"AB"partway through). This is likely unintended.Move the prefix handling before the loop:
+ if (len >= 2 && hex[0] == '0' && (hex[1] == 'x' || hex[1] == 'X')) { + i = 2; + } + while (i < len) { - if (hex[i] == '0' && (hex[i + 1] == 'x' || hex[i + 1] == 'X')) { - i += 2; - } - int temp = tolower(hex[i]);chapter_5/exercise_5_05/strncpy.c-17-21 (1)
17-21: Remove extraneous semicolon.Line 20 contains a standalone semicolon that serves no purpose and reduces code clarity.
Apply this diff:
void strcpy_ptr(char *s, char *t, size_t n) { while ((*s++ = *t++) && --n) ; - ; }chapter_1/exercise_1_15/temperature_conversion.c-5-25 (1)
5-25: Fix typo in comment.Line 13 contains a typo: "abouve" should be "above".
Apply this diff:
- // Printing a heading abouve the table + // Printing a heading above the tablechapter_5/exercise_5_09/date_conversion_pointers.c-70-78 (1)
70-78:yeardaylower bound not validated.The check validates upper bounds but doesn't reject
yearday <= 0. A zero or negative yearday would skip the loop entirely, returning*pmonth = 0and*pda = yearday, which are invalid results without any error indication.// Additional check for yearday. - if ((!leap && yearday > 365) || (leap && yearday > 366)) { + if (yearday < 1 || (!leap && yearday > 365) || (leap && yearday > 366)) { // Reset the provided month and day. *pmonth = 0; *pda = 0; - printf("Error: year %d has %d days.\n", year, leap ? 366 : 365); + printf("Error: yearday must be between 1 and %d for year %d.\n", + leap ? 366 : 365, year); return; }chapter_5/exercise_5_09/date_conversion_pointers.c-45-50 (1)
45-50: Month validation misses lower bound check.The validation only checks
month > 12but doesn't validatemonth < 1. Passingmonth = 0or negative values would accessdaytab[leap][0](the sentinel) or cause undefined behavior.// Additional checks for month. - if (month > 12) { + if (month < 1 || month > 12) { printf("Error: a year has 12 months, so please choose a number " "betweeen 1 and 12.\n"); return -1; }chapter_7/exercise_7_03/minprintf.c-36-39 (1)
36-39:%Xformat specifier prints lowercase instead of uppercase.The
case 'X'falls through tocase 'x'and uses"%x", so uppercase%Xin the format string produces lowercase hex output. To match standard printf behavior, handle them separately.case 'x': - case 'X': printf("%x", va_arg(arg_p, int)); break; + + case 'X': + printf("%X", va_arg(arg_p, int)); + break;chapter_7/exercise_7_03/minprintf.c-57-65 (1)
57-65:%Eand%Gformat specifiers also print lowercase.Same issue as
%X: thecase 'E'falls through tocase 'e'using"%e", andcase 'G'falls through tocase 'g'using"%g". Both should preserve the uppercase output format.case 'e': - case 'E': printf("%e", va_arg(arg_p, double)); break; + case 'E': + printf("%E", va_arg(arg_p, double)); + break; + case 'g': - case 'G': printf("%g", va_arg(arg_p, double)); break; + + case 'G': + printf("%G", va_arg(arg_p, double)); + break;chapter_5/exercise_5_16/sort.c-43-51 (1)
43-51: Empty input causes underflow inquick_sortcall.When
read_linesreturns 0 (no lines read),nr_of_lines - 1underflows toSIZE_MAX. Althoughquick_sorthandles this via the(long)start >= (long)endcheck, passingSIZE_MAXasendis semantically incorrect and relies on implementation details.if ((nr_of_lines = read_lines(line_ptr, MAX_NR_OF_LINES)) != (size_t)-1) { - quick_sort((void **)line_ptr, 0, nr_of_lines - 1, - (int (*)(void *, void *))comp); - write_lines(line_ptr, nr_of_lines); + if (nr_of_lines > 0) { + quick_sort((void **)line_ptr, 0, nr_of_lines - 1, + (int (*)(void *, void *))comp); + } + write_lines(line_ptr, nr_of_lines); } else {chapter_5/exercise_5_15/sort.c-43-50 (1)
43-50: Empty input causes underflow inquick_sortcall.Same issue as exercise_5_16: when
nr_of_linesis 0,nr_of_lines - 1underflows toSIZE_MAX. Guard with an empty check before callingquick_sort.if ((nr_of_lines = read_lines(line_ptr, MAX_NR_OF_LINES)) != (size_t)-1) { - quick_sort((void **)line_ptr, 0, nr_of_lines - 1, - (int (*)(void *, void *))comp); - write_lines(line_ptr, nr_of_lines); + if (nr_of_lines > 0) { + quick_sort((void **)line_ptr, 0, nr_of_lines - 1, + (int (*)(void *, void *))comp); + } + write_lines(line_ptr, nr_of_lines); } else {chapter_7/exercise_7_06/compare.c-34-48 (1)
34-48: Resource leak: files are not closed when files match completely.If the
whileloop completes without finding a mismatch (files are identical), neitherfile_1norfile_2is closed beforeexit(EXIT_SUCCESS). While the OS reclaims resources on exit, explicitly closing files is good practice and ensures buffers are flushed.++line_number; } + fclose(file_1); + fclose(file_2); exit(EXIT_SUCCESS); }chapter_4/exercise_4_06/variables.c-227-235 (1)
227-235: Logic issue: returningnextdiscards the minus sign and breaks token string.When
c == '-'andnextis neither whitespace, digit, nor '.', line 228 returnsnextdirectly. However,s[0]still contains'-', so subsequent error messages usingswill be misleading. The characternextshould be ungetch'd and'-'returned as an operator, or the token string should be updated.} else if (!isdigit(next) && next != '.') { - return next; // not a number + ungetch(next); + return c; // return '-' as operatorchapter_4/exercise_4_05/math.c-186-187 (1)
186-187: Same issue: returningnextdiscards the minus sign.Same pattern as other calculator files. When
-is followed by a non-numeric, non-whitespace character,nextis returned but the token stringsis inconsistent.} else if (!isdigit(next) && next != '.') { - return next; // not a number + ungetch(next); + return c; // return '-' as operatorchapter_4/exercise_4_03/calculator.c-122-123 (1)
122-123: Same issue: returningnextdiscards the minus sign.Same as in
variables.c: when-is followed by a non-numeric, non-whitespace character,nextis returned buts[0]still contains'-'. Consider ungetch'ingnextand returning'-'as the operator.} else if (!isdigit(next) && next != '.') { - return next; // not a number + ungetch(next); + return c; // return '-' as operatorchapter_6/exercise_6_01/count_c_keywords.c-121-125 (1)
121-125: Off-by-one: null terminator can write past buffer.If a word reaches exactly
max_word_lencharacters, the while loop exits withi == max_word_len, and Line 125 writesword[max_word_len] = '\0', which is one byte past the allocated buffer.- while ((isalnum(c = getc(stdin)) || c == '_') && i < max_word_len) { + while ((isalnum(c = getc(stdin)) || c == '_') && i < max_word_len - 1) { word[i++] = c; }chapter_5/exercise_5_20/dcl.c-125-133 (1)
125-133: Off-by-one: null terminator can write past buffer.If an identifier reaches exactly
max_lencharacters, the loop exits withi == max_len, and Line 131 writesdest[max_len] = '\0', which is out of bounds.- while ((isalnum(c = getc(stdin)) || c == '_') && i < max_len) { + while ((isalnum(c = getc(stdin)) || c == '_') && i < max_len - 1) { dest[i++] = c; }chapter_4/exercise_4_04/stack.c-167-178 (1)
167-178: Potential token loss when '-' is followed by non-numeric, non-whitespace character.When
-is followed by a character that is neither whitespace/newline nor a digit/dot (e.g.,-x), Line 173 returnsnextdirectly. This discards the-character that was stored ins[0], which may cause unexpected behavior for inputs like-abc.Consider whether the
-should be ungetch'd so subsequent calls can process it:} else if (!isdigit(next) && next != '.') { - return next; // not a number + ungetch(next); + return c; // return '-' as operator, put back next } else // number like "-5", "-.6" etc, next is digit or '.'chapter_5/exercise_5_18/dcl.c-93-101 (1)
93-101: Off-by-one risk whenmax_lencharacters are read.If exactly
max_lencharacters are read, the loop terminates butdest[i]writes the null terminator at indexmax_len, which is one past the valid range if the caller sized the buffer exactly tomax_len.Consider using
i < max_len - 1to reserve space for the null terminator:- while ((isalnum(c = getc(stdin)) || c == '_') && i < max_len) { + while ((isalnum(c = getc(stdin)) || c == '_') && i < max_len - 1) {chapter_6/exercise_6_06/define.c-170-179 (1)
170-179: Potential buffer overflow whenmax_str_lencharacters are read.Similar to
get_namein dcl.c, if exactlymax_str_lencharacters are read, the null terminator atstr[i]writes past the buffer.- while (isalnum(c = getc(stdin)) && i < max_str_len) { + while (isalnum(c = getc(stdin)) && i < max_str_len - 1) {chapter_5/exercise_5_06/itoa.c-32-43 (1)
32-43:itoafails forn = 0and negative numbers.The
while (n)loop condition means whennis 0, the loop body never executes, resulting in an empty string. Additionally, negative numbers will produce incorrect results due to the modulo behavior with negative operands.For a complete implementation:
void itoa(char *s, int n) { char *t = s; + int sign = n; + + if (n < 0) { + n = -n; + } + + do { + *(t++) = n % 10 + '0'; + n /= 10; + } while (n); - while (n) { - *(t++) = n % 10 + '0'; - n /= 10; - } + if (sign < 0) { + *(t++) = '-'; + } *t = '\0'; reverse(s); }chapter_6/exercise_6_06/define.c-161-165 (1)
161-165: Same buffer boundary issue inget_word.The loop can fill up to
max_word_lencharacters before null-terminating atword[i], risking a write past the buffer.- while ((isalnum(c = getc(stdin)) || c == '_') && i < max_word_len) { + while ((isalnum(c = getc(stdin)) || c == '_') && i < max_word_len - 1) {
🧹 Nitpick comments (38)
chapter_2/exercise_2_06/setbits.c (1)
24-33: Excellent fix for undefined behavior.Switching masks from signed 0 to unsigned 0U prevents undefined behavior when left-shifting. Left-shifting negative signed values is UB in C, and
~0(whenintis signed) produces a negative value. Using~0Uensures the mask operations are well-defined. The comment clearly documents this important safety improvement.Minor note: The function returns
unsigned intbut acceptsintparameters. While this works through implicit conversion, consider making parameter typesunsigned intas well for consistency. Given this is educational K&R exercise code, the current approach is acceptable.chapter_1/exercise_1_14/frequency_histogram.c (1)
27-31: Simplify the redundant condition.The
else if (i >= ALPHA_NR)condition on line 29 is redundant since we're already in the else block wherei < ALPHA_NRis false. Simplify to justelsefor clarity.Apply this diff:
if (i < ALPHA_NR) { printf("%c: ", 'a' + i); - } else if (i >= ALPHA_NR) { + } else { printf("%c: ", '0' + i - ALPHA_NR); }chapter_2/exercise_2_07/invert.c (2)
28-31: Consider simplifying with XOR.The current logic is correct but can be simplified. Since
mask1has 1s at positions to invert, XOR directly flips those bits:Apply this diff:
- unsigned int mask1 = ~(~0U << n) << p; - unsigned int mask2 = ~mask1 & x; - - return mask2 | ~x; + unsigned int mask = ~(~0U << n) << p; + return x ^ mask;This achieves the same result more concisely by XORing with the mask, which flips only the targeted bits.
23-24: Optional: Align parameter types with implementation.The function signature accepts
int xbut the implementation treats it as unsigned. Consider updating the signature for consistency:-unsigned int invert(int x, int p, int n) { - ++p; // First position is 0 +unsigned int invert(unsigned int x, unsigned int p, unsigned int n) { + ++p; // Adjust p for 0-based indexing (shift amount)Benefits:
- Eliminates implicit signed→unsigned conversion for
xunsigned intforpandnbetter represents valid bit positions (non-negative)- Clearer comment explains the adjustment
chapter_5/exercise_5_13/tail.c (2)
91-112: Consider usingSIZE_MAXor a sentinel value explicitly.Returning
-1from asize_tfunction relies on implicit cast toSIZE_MAX. While technically defined, it's less clear than usingSIZE_MAXexplicitly or redesigning the function to use a different error indicator (e.g., returnintstatus and use output parameter for count).Apply this diff to use
SIZE_MAXexplicitly:size_t read_lines(char *line_ptr[], const size_t max_nr_of_lines) { size_t line_length; size_t nr_of_lines = 0; char *current_line = alloc(MAX_LINE_LEN); char *current_line_copy = NULL; while ((line_length = get_line(current_line, MAX_LINE_LEN))) { if (nr_of_lines >= max_nr_of_lines || (current_line_copy = alloc(line_length)) == NULL) { - return -1; + return SIZE_MAX; } else { current_line[line_length - 1] = '\0'; strcpy(current_line_copy, current_line); line_ptr[nr_of_lines++] = current_line_copy; } } afree(current_line); return nr_of_lines; }And update the check in
main:- if ((total_nr_of_lines = read_lines(line_ptr, MAX_NR_OF_LINES)) != - (size_t)-1) { + if ((total_nr_of_lines = read_lines(line_ptr, MAX_NR_OF_LINES)) != + SIZE_MAX) { write_lines(line_ptr, nr_of_lines_to_print, total_nr_of_lines); } else {
128-137: Consider defensive check for pointer validity.The cast of
ptrdiff_ttosize_tat line 131 could mask bugs ifalloc_psomehow exceedsalloc_buf + ALLOC_SIZE. While this shouldn't happen in correct usage, a defensive check would improve robustness.Apply this diff:
char *alloc(size_t size) { - // Cast pointer difference to size_t to match unsigned size parameter - // This prevents sign comparison warnings - if ((size_t)(alloc_buf + ALLOC_SIZE - alloc_p) >= size) { + if (alloc_p >= alloc_buf && alloc_p <= alloc_buf + ALLOC_SIZE && + (size_t)(alloc_buf + ALLOC_SIZE - alloc_p) >= size) { alloc_p += size; return alloc_p - size; } return NULL; }chapter_5/exercise_5_10/expr.c (5)
8-8: Consider usingsize_tfor the stack pointer.While
unsigned charis sufficient for the currentSTACK_SIZEof 15, usingsize_tis more idiomatic for array indexing and avoids future issues ifSTACK_SIZEincreases. It also matches the type typically used for array indices in C.Based on learnings, JebediahS suggested this improvement in the PR discussion.
Apply this diff:
-unsigned char stack_pointer = 0; // points to next free slot +size_t stack_pointer = 0; // points to next free slot
15-15: Consider using an enum or bool for error tracking.The
char Errorvariable works, but using an enum for error codes or separateboolflags would improve code clarity and type safety.Example with enum:
+typedef enum { + ERROR_NONE = 0, + ERROR_INVALID_ARG = 1, + ERROR_STACK_MISMATCH = 2, + ERROR_INVALID_OP = 3, + ERROR_DIV_BY_ZERO = 4 +} ErrorCode; + - char Error = 0; + ErrorCode Error = ERROR_NONE;
106-117: Consider returning status instead of printing errors.The current implementation prints errors directly and returns
0.0, which is ambiguous since0.0is a valid stack value. This reduces the function's reusability and makes it difficult for callers to distinguish between success and failure.Based on learnings, JebediahS suggested using a status-based return pattern.
Apply this diff for a status-based approach:
-float pop(void) +int pop(float *result) { if (stack_pointer > 0) { // since stack_pointer points to the next free slot // it has to be decremented to point to a non-empty // place - return stack[--stack_pointer]; + *result = stack[--stack_pointer]; + return 1; // success } - printf("Error: the stack is empty.\n"); - return 0; + return 0; // failure: stack empty }Then update callers to check the return value:
- float number2 = pop(); - float number1 = pop(); + float number1, number2; + if (!pop(&number2) || !pop(&number1)) { + Error = 2; // stack underflow + continue; + }
119-126: Consider returning status for consistency.Similar to
pop(),push()should return a status code instead of printing errors directly. This improves reusability and allows callers to handle overflow conditions appropriately.Based on learnings, JebediahS suggested using a status-based return pattern.
Apply this diff for consistency with the suggested
pop()changes:-void push(float element) { +int push(float element) { if (stack_pointer < STACK_SIZE) { // starts filling at stack[0] stack[stack_pointer++] = element; + return 1; // success } else { - printf("Error: the stack is full.\n"); + return 0; // failure: stack full } }Then update callers:
- push(number); + if (!push(number)) { + printf("Error: stack overflow.\n"); + return EXIT_FAILURE; + }
14-104: Tests are needed to verify the stack fixes.As discussed in the PR comments, adding tests would help verify the off-by-one fix and prevent regressions. The conversation identified several testing approaches suitable for this K&R solutions repository.
Based on the PR discussion, would you like me to help generate test code? I can provide:
Shell script tests (
test_expr.sh) that invoke the compiled program with various argument combinations and verify output — following the pattern suggested by CodeRabbit for CLI programs.Test cases covering:
- Push/pop single item
- Push until full (15 elements)
- Pop until empty
- LIFO order verification
- Edge cases: empty arguments, missing operators, division by zero
- Zero value handling: "0", "0.0", "-0"
Let me know if you'd like me to open a new issue or provide the test implementation directly.
chapter_3/exercise_3_06/itoa.c (1)
1-1: Consider removing unused include.
<limits.h>doesn't appear to be used in this file—abs()comes from<stdlib.h>.-#include <limits.h>chapter_2/exercise_2_09/bitcount.c (1)
24-26: Prefer bit-shift overpow()for bit operations.Using
pow(2, i)involves floating-point arithmetic, which is slower and can introduce precision issues. Use bit-shift for integer operations:- (x & (unsigned int)pow(2, i)) ? putchar('1') : putchar('0'); + (x & (1U << i)) ? putchar('1') : putchar('0');This also removes the dependency on
<math.h>for this function.chapter_2/exercise_2_10/lower.c (1)
13-13: Consider using addition instead of compound assignment in ternary.The expression
c += 'a' - 'A'works but unnecessarily mutates the parameter. A cleaner form would be:-char lower(char c) { return (c >= 'A' && c <= 'Z') ? c += 'a' - 'A' : c; } +char lower(char c) { return (c >= 'A' && c <= 'Z') ? c + ('a' - 'A') : c; }This avoids the side effect and is clearer in intent.
chapter_1/exercise_1_22/fold_line.c (1)
65-69: Potential edge case withlast_blankcheck.Using
if (last_blank)treats index 0 as "no blank found." While this is safe here sincesplitonly becomes TRUE at column 30+, makinglast_blankat index 0 impossible, consider using a sentinel value like-1for clarity:- int last_blank = 0; + int last_blank = -1; ... - if (last_blank) { + if (last_blank >= 0) { fold_str[last_blank] = '\n'; column = j - last_blank; - last_blank = 0; + last_blank = -1;chapter_1/exercise_1_23/c_remove_comments.c (1)
76-80: Redundant condition on line 78.The
else if (!line_comment || !block_comment)condition is always true when reached, since the precedingifalready handles the case where either flag is set. This can be simplified toelse:if (line_comment || block_comment) { ++i; - } else if (!line_comment || !block_comment) { + } else { no_com_str[j++] = str[i++]; }chapter_5/exercise_5_14/sort.c (1)
68-71: Unreachable code afterreturn.The
breakon line 70 is dead code since it follows areturnstatement.default: return 0; - break; }chapter_4/exercise_4_08/getch.c (1)
19-30: Consider usingintfortempto matchbufand return type.
tempis declared ascharbutbufisintandgetchreturnsint. For characters with values > 127 (extended ASCII), assigning tocharmay cause sign extension issues on some platforms.int getch(void) { - char temp; + int temp; if (buf != -1) { temp = buf;chapter_6/exercise_6_02/var_group.c (1)
94-100: Consider adding null checks after malloc.The
malloccalls instr_dup(line 95),add_to_tree(line 201), andadd_to_list(line 225) don't verify allocation success before dereferencing. While this appears to be a pre-existing pattern in the codebase, adding null checks would improve robustness.Example for
str_dup:char *str_dup(char *src) { char *dest = (char *)malloc(strlen(src) + 1); - if (dest != NULL) { + if (dest == NULL) { + return NULL; + } - strcpy(dest, src); - } + strcpy(dest, src); return dest; }Similar checks should be added for
add_to_treeandadd_to_list.Also applies to: 197-235
chapter_7/exercise_7_03/minprintf.c (1)
41-43:%ushould extractunsigned intfrom va_list.The
%uformat specifier is for unsigned integers, butva_arg(arg_p, int)extracts a signed int. This can produce incorrect output for large unsigned values.case 'u': - printf("%u", va_arg(arg_p, int)); + printf("%u", va_arg(arg_p, unsigned int)); break;chapter_2/exercise_2_08/rightrot.c (1)
24-26: Prefer bit shift overpow()for bit extraction.Using
pow(2, i)involves floating-point arithmetic, which is slower and can introduce precision issues for large values. Use(1U << i)instead for integer bit operations.int i; for (i = n * 8 - 1; i >= 0; --i) { - (x & (unsigned int)pow(2, i)) ? putchar('1') : putchar('0'); + (x & (1U << i)) ? putchar('1') : putchar('0'); }This also allows removing the
#include <math.h>dependency.chapter_5/exercise_5_16/sort.c (1)
78-81: Unreachablebreakafterreturn.The
breakon line 80 is never executed sincereturn 0exits the function first.default: return 0; - break; }chapter_5/exercise_5_15/sort.c (1)
73-76: Unreachablebreakafterreturn.default: return 0; - break; }chapter_7/exercise_7_06/compare.c (1)
25-28: Minor:file_1not closed iffile_2fails to open.If opening
file_2fails,file_1remains open when the program exits. Consider closingfile_1before exiting for consistency.if ((file_2 = fopen(argv[2], "r")) == NULL) { fprintf(stderr, "%s: can't open %s.\n", program_name, argv[2]); + fclose(file_1); exit(EXIT_FAILURE); }chapter_4/exercise_4_03/calculator.c (1)
59-61: Result printed even when stack is empty.On newline,
pop()is always called. If the stack is empty, this prints "Error: stack empty." followed by "result: 0" which is confusing. Consider checking if the stack is empty first, similar tochapter_4/exercise_4_06/variables.clines 120-124.case '\n': - printf("result: %.8g\n", pop()); + if (sp > 0) { + printf("result: %.8g\n", pop()); + } break;chapter_4/exercise_4_05/math.c (1)
94-96: Result printed even when stack is empty.Same issue as
chapter_4/exercise_4_03/calculator.c: on newline,pop()is called unconditionally. This prints an error message followed by a misleading "result: 0" if the stack is empty.case '\n': - printf("result: %.8g\n", pop()); + if (sp > 0) { + printf("result: %.8g\n", pop()); + } break;chapter_4/exercise_4_04/stack.c (1)
122-134: Consider guardingduplicate()andswap()against empty/underflow.Both functions call
pop()without checking stack state first. If the stack is empty or has insufficient elements,pop()prints an error but returns 0.0, which then gets pushed—silently corrupting the stack state.void duplicate(void) { + if (sp < 1) { + printf("Error: stack empty, can't duplicate.\n"); + return; + } double temp = pop(); push(temp); push(temp); } void swap(void) { + if (sp < 2) { + printf("Error: stack needs at least 2 elements to swap.\n"); + return; + } double temp1 = pop(); double temp2 = pop(); push(temp1); push(temp2); }chapter_5/exercise_5_12/entab.c (2)
86-93: Empty string returns true;strlen()called repeatedly in loop.
An empty string
""passes validation since the loop never executes, returning 1. This could accept invalid arguments like./entab +(whereargv[i]+1is empty).
strlen(str)is called on every iteration, making this O(n²). Consider caching or using a pointer-based approach.int is_str_uint(char *str) { - for (size_t i = 0; i < strlen(str); ++i) { - if (!isdigit(str[i])) { + if (*str == '\0') { + return 0; // empty string is not a valid uint + } + for (; *str != '\0'; ++str) { + if (!isdigit((unsigned char)*str)) { return 0; } } return 1; }
54-57: Complex condition may benefit from clarifying comments.The tab insertion condition spans multiple lines with three distinct cases. A brief inline comment explaining each case would improve readability for future readers.
chapter_6/exercise_6_01/count_c_keywords.c (1)
23-33: Note:size_tis a typedef, not a C keyword.The keyword table includes
size_t, which is technically a typedef from<stddef.h>rather than a reserved C keyword. This is a minor inaccuracy in the educational context.chapter_5/exercise_5_20/dcl.c (1)
221-239: Unboundedstrcat()onout[]could overflow.The
outbuffer (1000 chars) is concatenated repeatedly without bounds checking. While unlikely with typical input, deeply nested declarations or many function parameters could overflow the buffer.Consider using
snprintf()or tracking the remaining buffer size to prevent potential overflow in edge cases.chapter_5/exercise_5_06/getop.c (1)
33-35: Redundant condition:c != '.'is unnecessary.Since
'.'is not a digit,isdigit(c)already excludes it. The&& c != '.'part is redundant.- if (isdigit(c) && c != '.') + if (isdigit(c)) while (isdigit(*(++s) = c = getchar())) ;.github/workflows/c.yml (1)
1-22: CI workflow looks good; consider adding a test step.The workflow structure is sound: format checking and linting run before the build, which is the correct order. Given the PR discussion mentions adding tests to the repository, consider adding a test step once tests are available:
- name: Build run: make + + - name: Test + run: make testThis would require a corresponding
testtarget in the Makefile.chapter_1/exercise_1_20/detab.c (1)
10-16: Tab expansion uses fixed 8 spaces instead of position-aware expansion.The current implementation always outputs 8 spaces per tab, regardless of the current column position. True tab behavior expands to the next tab stop (e.g., at column 3, a tab would insert 5 spaces to reach column 8). For K&R exercise 1-20, this simplified approach may be acceptable if the exercise doesn't require position-aware expansion.
If position-aware expansion is desired:
int main(void) { int c; - unsigned int nr_of_spaces; + unsigned int line_pos = 0; while ((c = getchar()) != EOF) { if (c == '\t') { - nr_of_spaces = TAB_LENGTH; - - while (nr_of_spaces) { + do { putchar(' '); - --nr_of_spaces; - } + ++line_pos; + } while (line_pos % TAB_LENGTH != 0); } else { putchar(c); + if (c == '\n') { + line_pos = 0; + } else { + ++line_pos; + } } }Makefile (2)
26-31:install-hooksassumesscripts/pre-commitexists without verification.If
scripts/pre-commitdoesn't exist, thecpcommand will fail silently (depending on shell behavior) or with an error. Consider adding a check for the source file.install-hooks: - @if [ -d .git ] && [ ! -f .git/hooks/pre-commit ]; then \ + @if [ -d .git ] && [ ! -f .git/hooks/pre-commit ] && [ -f scripts/pre-commit ]; then \ cp scripts/pre-commit .git/hooks/pre-commit; \ chmod +x .git/hooks/pre-commit; \ echo "Pre-commit hook installed."; \ fi
40-40: Consider adding atesttarget placeholder.As discussed in the PR comments, the repository is exploring testing approaches. Adding a placeholder
testtarget now would make CI integration smoother later.+test: + @echo "No tests configured yet. See PR discussion for testing approaches." + -.PHONY: all lint format format-check check install-hooks clean +.PHONY: all lint format format-check check install-hooks clean testchapter_6/exercise_6_04/words_frequency.c (1)
134-151: Consider adding malloc failure handling.Lines 138-141 allocate and immediately use a
tree_nodewithout checking ifmallocreturnedNULL. If allocation fails, this leads to null pointer dereferences.Apply this diff to add proper error handling:
if (node_p == NULL) { node_p = (struct tree_node *)malloc(sizeof(struct tree_node)); + if (node_p == NULL) { + fprintf(stderr, "Error: memory allocation failed\n"); + exit(EXIT_FAILURE); + } node_p->word = str_dup(word); + if (node_p->word == NULL) { + free(node_p); + fprintf(stderr, "Error: memory allocation failed\n"); + exit(EXIT_FAILURE); + } node_p->count = 1; node_p->left = node_p->right = NULL;Based on static analysis hints.
chapter_5/exercise_5_12/detab.c (1)
81-88: Optionally cache strlen result in loop.Line 82 calls
strlen(str)on every iteration. For better performance, consider caching the length:int is_str_uint(char *str) { - for (size_t i = 0; i < strlen(str); ++i) { + size_t len = strlen(str); + for (size_t i = 0; i < len; ++i) { if (!isdigit(str[i])) { return 0; } } return 1; }Note: The same pattern exists in
entab.c(shown in relevant snippets), so consider applying this optimization to both files.
| char c; | ||
| int word_count_index = 0; | ||
| while ((c = getchar())) { | ||
| if (c == ' ' || c == '\t' || c == '\n' || c == EOF) { |
There was a problem hiding this comment.
Fix EOF handling by using int type for getchar().
The variable c is declared as char on line 22, but getchar() returns int to distinguish EOF from valid byte values. Comparing a char with EOF (typically -1) is undefined behavior and fails on platforms where char is unsigned. This causes the EOF check on line 25 to malfunction, potentially leading to an infinite loop or incorrect termination.
Apply this diff to fix the type and loop condition:
// Count the words length and store in histogram array at the
// specific index
- char c;
+ int c;
int word_count_index = 0;
- while ((c = getchar())) {
+ while ((c = getchar()) != EOF) {
if (c == ' ' || c == '\t' || c == '\n' || c == EOF) {Note: After this fix, the explicit c == EOF check on line 25 becomes redundant with the loop condition, but keeping it doesn't harm correctness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| char c; | |
| int word_count_index = 0; | |
| while ((c = getchar())) { | |
| if (c == ' ' || c == '\t' || c == '\n' || c == EOF) { | |
| int c; | |
| int word_count_index = 0; | |
| while ((c = getchar()) != EOF) { | |
| if (c == ' ' || c == '\t' || c == '\n' || c == EOF) { |
🤖 Prompt for AI Agents
In chapter_1/exercise_1_13/histogram.c around lines 22 to 25, the variable c is
declared as char but getchar() returns int so EOF cannot be reliably detected;
change c's type to int and update the loop to while ((c = getchar()) != EOF)
(you may keep or remove the explicit c == EOF check inside the loop since the
loop condition already handles EOF), ensuring all comparisons to EOF use the int
variable.
| if (x == v[low - 1]) { | ||
| return low - 1; | ||
| } | ||
|
|
||
| return -1; | ||
| return -1; |
There was a problem hiding this comment.
Out-of-bounds array access when low is 0.
When searching for a value smaller than all elements, the loop terminates with low == 0. The expression v[low - 1] then accesses v[-1], which is undefined behavior.
The test on line 22 uses x = -1, which triggers this exact condition since all vector elements are non-negative.
Add a bounds check:
- if (x == v[low - 1]) {
+ if (low > 0 && x == v[low - 1]) {
return low - 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (x == v[low - 1]) { | |
| return low - 1; | |
| } | |
| return -1; | |
| return -1; | |
| if (low > 0 && x == v[low - 1]) { | |
| return low - 1; | |
| } | |
| return -1; |
🤖 Prompt for AI Agents
In chapter_3/exercise_3_01/binsearch.c around lines 84 to 88, the final check
uses v[low - 1] which is out-of-bounds when low == 0; change the logic to first
verify low > 0 before accessing v[low - 1], returning low - 1 only when low > 0
and v[low - 1] == x, otherwise return -1.
| void clear(void) { | ||
| do { | ||
| stack[sp] = 0.0; | ||
| } while (sp--); | ||
| } |
There was a problem hiding this comment.
Critical: clear() causes stack pointer underflow.
The do-while loop always executes at least once and decrements sp unconditionally. When sp == 0, this sets sp = -1, causing subsequent push() calls to write to stack[-1] (memory corruption).
void clear(void) {
- do {
- stack[sp] = 0.0;
- } while (sp--);
+ while (sp > 0) {
+ stack[--sp] = 0.0;
+ }
}🤖 Prompt for AI Agents
In chapter_4/exercise_4_10/calculator.c around lines 189 to 193, clear() uses a
do-while that always decrements sp and can underflow when sp == 0; replace the
do-while with a loop that only runs while sp >= 0 (for example: while (sp >= 0)
{ stack[sp] = 0.0; sp--; } or iterate from i = sp down to 0 to zero elements and
then set sp = -1) so sp never becomes negative and the stack is properly
cleared.
| int getop(char s[]) { | ||
| int i = 0, c; | ||
| static int buf = EOF; | ||
|
|
||
| while ((s[0] = c = getchar()) == ' ' || c == '\t') | ||
| ; | ||
|
|
||
| s[1] = '\0'; | ||
|
|
||
| while ((s[0] = c = getchar()) == ' ' || c == '\t') | ||
| ; | ||
| if (!isdigit(c) && c != '.' && c != '-') { | ||
| return c; | ||
| } | ||
|
|
||
| if (c == '-') { | ||
| int next = getchar(); | ||
| if (!isdigit(next) && next != '.') { | ||
| return next; | ||
| } | ||
|
|
||
| s[i] = c; | ||
| c = next = buf; | ||
| } else { | ||
| c = getchar(); | ||
| } | ||
|
|
||
| s[1] = '\0'; | ||
| if (isdigit(c)) { | ||
| while (isdigit(s[++i] = c = getchar())) | ||
| ; | ||
| } | ||
|
|
||
| if (!isdigit(c) && c != '.' && c != '-') | ||
| { | ||
| return c; | ||
| } | ||
| if (c == '.') { | ||
| while (isdigit(s[++i] = c = getchar())) | ||
| ; | ||
| } | ||
|
|
||
| if (c == '-') | ||
| { | ||
| int next = getchar(); | ||
| if (!isdigit(next) && next != '.') | ||
| { | ||
| return next; | ||
| if (c != EOF) { | ||
| buf = c; | ||
| } | ||
|
|
||
| s[i] = c; | ||
| c = next = buf; | ||
| } | ||
| else | ||
| { | ||
| c = getchar(); | ||
| } | ||
|
|
||
| if (isdigit(c)) | ||
| { | ||
| while (isdigit(s[++i] = c = getchar())) | ||
| ; | ||
| } | ||
|
|
||
| if (c == '.') | ||
| { | ||
| while (isdigit(s[++i] = c = getchar())) | ||
| ; | ||
| } | ||
|
|
||
| if (c != EOF) | ||
| { | ||
| buf = c; | ||
| } | ||
|
|
||
| return NUMBER; | ||
| return NUMBER; | ||
| } |
There was a problem hiding this comment.
Critical logic errors in negative number handling.
The negative number parsing has several issues:
- Line 108: Returns
nextdirectly without buffering it, causing the character to be lost. - Line 112: The assignment
c = next = bufis incorrect. It should buffernextintobufand then usenextas the current character. Currently, it assignsEOFto both variables. - The logic flow between lines 105-115 is fundamentally broken.
The correct approach should:
- After reading the minus sign, check the next character
- If next is not a digit/period, return minus and buffer next
- If next is a digit/period, continue parsing the number
Apply this diff to fix the logic:
if (c == '-') {
int next = getchar();
if (!isdigit(next) && next != '.') {
- return next;
+ if (next != EOF) {
+ buf = next;
+ }
+ return c;
}
- s[i] = c;
- c = next = buf;
+ s[++i] = c;
+ c = next;
} else {
c = getchar();
}🤖 Prompt for AI Agents
In chapter_4/exercise_4_11/getop.c around lines 92-132 the negative-number
handling is wrong: after reading '-' you must peek the next char and if it is
not a digit or '.' you must buffer that peeked char into buf and return '-' (not
the peeked char); if it is a digit or '.' you must treat '-' as the start of a
number by storing '-' and the peeked char into s (set i appropriately), set c to
the peeked char to continue numeric parsing, and do NOT overwrite these with EOF
— implement buffering as buf = next when you need to save the peeked char, and
set c = next when you continue parsing so the subsequent while loops read the
rest of the number correctly.
| while (*s-- == *t--) | ||
| --t_length; | ||
|
|
||
| return !t_length; | ||
| return !t_length; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n chapter_5/exercise_5_04/strend.cRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 1605
🏁 Script executed:
find . -name "strend.c" -type fRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Find and display the strend.c file with full context
if [ -f "chapter_5/exercise_5_04/strend.c" ]; then
echo "=== File found: chapter_5/exercise_5_04/strend.c ==="
cat -n chapter_5/exercise_5_04/strend.c
else
echo "File not found at expected path. Searching for it..."
find . -name "strend.c" -type f 2>/dev/null
fiRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 1658
Critical: Buffer underflow in backward string comparison loop.
The while condition at line 39 lacks a guard for when t_length reaches zero. After matching all characters in the shorter string t, the loop continues if the current characters still match, causing pointer t to decrement into invalid memory offsets, resulting in undefined behavior.
Example failure case:
strend("test", "st")should return 1- After first match ('t'=='t'): t moves from index 1 to 0, t_length becomes 1
- After second match ('s'=='s'): t moves from index 0 to -1 (out of bounds) before t_length is decremented to 0
- Third iteration attempts comparison with memory before the buffer
Add the t_length boundary check:
- while (*s-- == *t--)
+ while (t_length > 0 && *s-- == *t--)
--t_length;This ensures the loop exits immediately when all characters in t have been matched, preventing out-of-bounds pointer access.
🤖 Prompt for AI Agents
In chapter_5/exercise_5_04/strend.c around lines 39 to 42, the backward
comparison loop can decrement t past the start of the buffer causing undefined
behavior; modify the loop to check that t_length is greater than zero before
doing the character comparison so the loop exits immediately when all characters
of t have been matched (i.e., add a t_length guard in the while condition),
preserving the existing pointer semantics and the final return of !t_length.
| char *p = | ||
| stored_lines + strlen(stored_lines); // Init p with the first empty | ||
| // position from stored_lines |
There was a problem hiding this comment.
Calling strlen on uninitialized memory causes undefined behavior.
This line calls strlen on the stored_lines buffer, which is passed in uninitialized from main. The function will read arbitrary memory until it encounters a null byte, leading to unpredictable behavior.
Based on the comment, the intent is to support appending to an existing buffer, but the caller must ensure the buffer is properly initialized (either zeroed or null-terminated).
The fix in the previous comment (initializing stored_lines in main) will resolve this issue.
🧰 Tools
🪛 Cppcheck (2.18.0)
[warning] 39-39: Uninitialized variable
(uninitvar)
🤖 Prompt for AI Agents
In chapter_5/exercise_5_07/readlines.c around lines 38 to 40, the code calls
strlen(stored_lines) on a buffer that may be uninitialized, causing undefined
behavior; fix by ensuring the buffer is null-terminated before using strlen
(e.g., initialize stored_lines[0] = '\0' or memset the buffer to 0 in main) or
change the function signature to accept and use a known current length instead
of calling strlen on caller-provided memory.
| if (node_p == NULL) { | ||
| node_p = (struct tree_node *)malloc(sizeof(struct tree_node)); | ||
| node_p->line_numbers = add_to_list(node_p->line_numbers, line_number); | ||
| node_p->word = str_dup(word); | ||
| node_p->left = node_p->right = NULL; |
There was a problem hiding this comment.
Use of uninitialized node_p->line_numbers and missing NULL check after malloc.
On line 131, node_p->line_numbers is passed to add_to_list before being initialized. Since malloc doesn't zero memory, this passes garbage to add_to_list, which then checks if (list_node_p == NULL). This is undefined behavior.
Additionally, if malloc returns NULL, the subsequent dereferences on lines 131-133 will crash.
Initialize line_numbers to NULL and add a null check:
if (node_p == NULL) {
node_p = (struct tree_node *)malloc(sizeof(struct tree_node));
+ if (node_p == NULL) {
+ return NULL;
+ }
+ node_p->line_numbers = NULL;
node_p->line_numbers = add_to_list(node_p->line_numbers, line_number);
node_p->word = str_dup(word);
node_p->left = node_p->right = NULL;🧰 Tools
🪛 Cppcheck (2.18.0)
[warning] 131-131: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 131-131: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 132-132: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 133-133: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 133-133: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
In chapter_6/exercise_6_03/cross_referencer.c around lines 129 to 133, malloc'd
node_p is not NULL-checked and node_p->line_numbers is used uninitialized;
change the allocation handling to check if malloc returned NULL and handle the
error (return, print and exit, or propagate) and explicitly initialize
node_p->line_numbers = NULL before calling add_to_list so add_to_list receives a
valid NULL list pointer; also continue to set node_p->word, left and right after
successful allocation.
| void quick_sort(void *v[], size_t start, size_t end, | ||
| int (*comp)(void *, void *)) { | ||
| if ((long)start >= (long)end) { | ||
| return; | ||
| } | ||
|
|
||
| swap(v, start, (start + end) / 2); | ||
|
|
||
| size_t last = start; | ||
| for (size_t i = start + 1; i <= end; ++i) { | ||
| if ((*comp)(v[i], v[start]) < 0) { | ||
| swap(v, ++last, i); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| swap(v, start, last); | ||
| quick_sort(v, start, last - 1, comp); | ||
| quick_sort(v, last + 1, end, comp); | ||
| swap(v, start, last); | ||
| quick_sort(v, start, last - 1, comp); | ||
| quick_sort(v, last + 1, end, comp); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current version of the function
echo "=== Current function (quick_sort) ==="
grep -n "void quick_sort" chapter_6/exercise_6_04/words_frequency.c
# Show the full current function
sed -n '114,132p' chapter_6/exercise_6_04/words_frequency.c
echo ""
echo "=== Checking git history ==="
# Check if the file was in previous commit
git log --oneline chapter_6/exercise_6_04/words_frequency.c | head -5
echo ""
echo "=== Previous version of quick_sort ==="
# Show the function from the previous commit
git show HEAD~1:chapter_6/exercise_6_04/words_frequency.c 2>/dev/null | sed -n '/void quick_sort/,/^}/p' || echo "Previous version not available"
echo ""
echo "=== Analyzing the potential underflow path ==="
# Show lines around the recursive call
sed -n '120,132p' chapter_6/exercise_6_04/words_frequency.c | cat -nRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 1283
🏁 Script executed:
#!/bin/bash
echo "=== Checking if there are multiple quick_sort implementations ==="
grep -n "void quick_sort" chapter_6/exercise_6_04/words_frequency.c
echo ""
echo "=== First quick_sort implementation (around line 29) ==="
sed -n '29,50p' chapter_6/exercise_6_04/words_frequency.c
echo ""
echo "=== Checking full git log ==="
git log --oneline | head -10
echo ""
echo "=== Analyzing underflow path logic ==="
# The issue occurs when: last == start (no swaps happened in loop)
# Then last - 1 where start is 0 -> underflows to SIZE_MAX
# Check if this scenario is possible
cat << 'EOF'
Logic analysis:
- last starts at: start
- last can only increase or stay same (incremented in swap condition)
- If no elements < v[start], last stays equal to start
- If start == 0, then last - 1 underflows to SIZE_MAX
- The cast to (long) in the guard would then convert SIZE_MAX to -1
- Guard check (long)0 >= (long)SIZE_MAX becomes 0 >= -1, which is TRUE
- So recursion stops, but this is UB-dependent behavior
EOFRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 1501
🏁 Script executed:
#!/bin/bash
echo "=== Checking the initial call to quick_sort ==="
sed -n '40,50p' chapter_6/exercise_6_04/words_frequency.c
echo ""
echo "=== Check how nr_of_nodes is used ==="
grep -n "nr_of_nodes" chapter_6/exercise_6_04/words_frequency.c | head -10
echo ""
echo "=== Verify: Can start == 0 and last == start after partitioning? ==="
cat << 'EOF'
Scenario verification:
1. Initial call: quick_sort(tree_node_list, 0, nr_of_nodes - 1, comp)
- start = 0, end = nr_of_nodes - 1
2. If nr_of_nodes == 1:
- Guard check: (long)0 >= (long)0 returns TRUE, so function returns early (SAFE)
3. If nr_of_nodes == 2:
- start = 0, end = 1
- Guard: (long)0 >= (long)1 is FALSE, continue
- swap(v, 0, (0+1)/2 = 0) -> no change
- Loop: i from 1 to 1
- If v[1] >= v[0], last stays 0
- swap(v, 0, 0) -> no change
- Recursive call 1: quick_sort(v, 0, 0-1, comp)
- start = 0, end = SIZE_MAX (UNDERFLOW!)
- Guard: (long)0 >= (long)SIZE_MAX
- SIZE_MAX as signed long = -1 (2's complement)
- Result: 0 >= -1 is TRUE, so returns (doesn't crash but UB)
This scenario IS reachable in practice!
EOFRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 1630
🏁 Script executed:
#!/bin/bash
echo "=== Additional verification: initial call with nr_of_nodes == 0 ==="
cat << 'EOF'
Initial call: quick_sort(tree_node_list, 0, nr_of_nodes - 1, comp)
If nr_of_nodes == 0:
- start = 0, end = SIZE_MAX (0 - 1 with size_t underflow!)
- Guard check: (long)0 >= (long)SIZE_MAX
- SIZE_MAX as signed long = -1
- Result: 0 >= -1 is TRUE, returns early (masked by guard)
This shows the guard check itself relies on undefined behavior to prevent crashes.
EOF
echo ""
echo "=== Conclusion ==="
cat << 'EOF'
VERIFIED: The size_t underflow bug is real and reachable.
1. Underflow occurs at line 130: quick_sort(v, start, last - 1, comp)
2. Scenario: When last == start (no partitioning happened)
3. If start == 0, then last - 1 underflows to SIZE_MAX
4. The guard check masks the bug by relying on undefined behavior
5. This is reachable in practice (e.g., 2-element arrays, or initial call with empty array)
The bug should be fixed by checking last > 0 before the recursive call, or by using signed integers consistently.
EOFRepository: ohkimur/the-c-programming-language-2nd-edition-solutions
Length of output: 1066
Fix size_t underflow in recursive call at line 130.
The expression last - 1 where last is size_t underflows to SIZE_MAX when last == start (which occurs when no elements are less than the pivot). This is reachable in practice with 2-element arrays or when the initial call has an empty array. While the guard check masks the crash through undefined behavior, it relies on implementation-specific casting rather than proper bounds checking.
Fix: Add a bounds check before the recursive call:
swap(v, start, last);
- quick_sort(v, start, last - 1, comp);
+ if (last > 0) {
+ quick_sort(v, start, last - 1, comp);
+ }
quick_sort(v, last + 1, end, comp);🤖 Prompt for AI Agents
In chapter_6/exercise_6_04/words_frequency.c around lines 114 to 132, the
recursive call quick_sort(v, start, last - 1, comp) can underflow because last
is size_t and may equal start; change the logic to perform an explicit bounds
check (e.g., only call quick_sort on the left partition if last > start) instead
of relying on signed casts, and keep the existing call for the right partition
as-is; also ensure the initial guard uses proper start >= end check with
size_t-safe comparisons.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
chapter_5/exercise_5_10/expr.c (4)
8-8: Consider usingsize_tforstack_pointer.While
unsigned charworks for the currentSTACK_SIZEof 15, usingsize_tis more idiomatic for array indices and provides better portability and future extensibility.Apply this diff to use
size_t:-unsigned char stack_pointer = 0; // points to next free slot +size_t stack_pointer = 0; // points to next free slotYou'll also need to update the comparison on line 26:
- if (stack_pointer >= 2 && stack_pointer < STACK_SIZE) { + if (stack_pointer >= 2) {The upper bound check is redundant since
pushalready guards against overflow.
21-24: Zero-value parsing improved but still has edge cases.The current fix (line 23) is better than the previous version which only checked
strcmp(argv[i], "0"). However, it still accepts malformed inputs like"0abc"as valid zero values becauseatofreturns0.0and the first character'0'is in the valid set.Consider using
strtofwith end-pointer validation for robust parsing:- float number = atof(argv[i]); + char *endptr; + float number = strtof(argv[i], &endptr); - if (number != 0.0 || strchr("-+.0123456789", argv[i][0]) != NULL) { + // Valid number if conversion consumed the entire string + if (endptr != argv[i] && *endptr == '\0') { push(number);
26-26: Remove redundant upper bound check.The condition
stack_pointer < STACK_SIZEis redundant. Ifstack_pointer >= 2, we already know we're belowSTACK_SIZEsince valid values are 0 to STACK_SIZE. The lower bound check alone is sufficient to ensure two operands are available for the operation.- if (stack_pointer >= 2 && stack_pointer < STACK_SIZE) { + if (stack_pointer >= 2) {
106-117: Stack pop logic is correct!The pre-decrement approach correctly implements the "next free slot" semantics and fixes the off-by-one bug. The explanatory comment is helpful.
Note: Returning
0on error is ambiguous when0.0is a valid stack value. Consider returning a status code via a return value and using an out-parameter for the popped value, though this would be a larger refactor.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
chapter_5/exercise_5_10/expr.c(1 hunks)chapter_5/exercise_5_10/expr_test.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
chapter_5/exercise_5_10/expr.c (1)
chapter_7/exercise_7_05/calculator.c (2)
pop(77-84)push(69-75)
🪛 Shellcheck (0.11.0)
chapter_5/exercise_5_10/expr_test.sh
[warning] 79-79: 'expr' expects 3+ arguments, but sees 2. Make sure each operator/operand is a separate argument, and escape <>&|.
(SC2307)
🔇 Additional comments (8)
chapter_5/exercise_5_10/expr.c (2)
1-7: LGTM!The includes are appropriate for the functionality used, and the stack size is reasonable for a calculator exercise.
119-128: Stack push logic is correct!The post-increment approach correctly implements the "next free slot" semantics and fixes the off-by-one bug. The comment clearly documents the behavior.
chapter_5/exercise_5_10/expr_test.sh (6)
1-12: LGTM! File existence checks are correct.The error message on line 5 is now correct (previously flagged in past review as inverted). Both presence and executability checks are properly implemented.
16-34: LGTM! Basic addition tests are correct.Both tests properly validate addition and commutativity using RPN notation, with expected output matching the format from
expr.c.
36-44: LGTM! Subtraction test is correct.The test correctly validates RPN subtraction:
10 3 -evaluates to10 - 3 = 7.
46-66: LGTM! Excellent complex expression test with detailed documentation.The step-by-step stack evolution comments (lines 48-59) make it easy to verify the correctness of the complex RPN expression. The proper escaping of
*is noted on line 60.
68-75: LGTM! Division by zero test is correct.The test properly validates that division by zero is detected and reported, using substring matching to allow flexibility in the exact error message format.
77-85: Test correctly validates missing operator error message.The test properly checks that two operands without an operator trigger the appropriate error message.
Note on shellcheck warning: The SC2307 warning is a false positive. Shellcheck assumes this is the shell built-in
exprcommand, but the test is actually invoking the custom./exprexecutable which has different argument requirements.Consider verifying exit codes: The tests currently only check output messages but don't verify exit codes. Since error cases should return
EXIT_FAILURE, consider adding exit code checks:# Test 6: Missing operator T=6 ./expr 5 1 exit_code=$? result=$(./expr 5 1) expected="Error: Operator is missing!" if [[ "$result" == "$expected" && $exit_code -ne 0 ]]; then echo "Test $T PASS: Missing operator" else echo "Test $T FAIL: Expected '$expected' with non-zero exit, got '$result' with exit code $exit_code" fiHowever, this would require fixing the exit code issue in
expr.cfirst (see review comment on lines 97-103).
| if (op_count == 0 && argc > 2) { | ||
| printf("Error: Operator is missing!"); | ||
| } else { | ||
| printf("result: %.3f", pop()); | ||
| } | ||
|
|
||
| return EXIT_SUCCESS; | ||
| return EXIT_SUCCESS; |
There was a problem hiding this comment.
Fix inconsistent error handling for missing operator.
The missing operator check (line 97-99) prints an error but still returns EXIT_SUCCESS, which is inconsistent with other error handling. Additionally:
- The error message lacks a trailing newline.
- If multiple operands are pushed without operators,
pop()only retrieves the top value, leaving others on the stack.
if (op_count == 0 && argc > 2) {
- printf("Error: Operator is missing!");
+ printf("Error: Operator is missing!\n");
+ return EXIT_FAILURE;
} else {
printf("result: %.3f", pop());
}
return EXIT_SUCCESS;Alternatively, validate that exactly one value remains on the stack:
if (op_count == 0 && argc > 2) {
- printf("Error: Operator is missing!");
- } else {
- printf("result: %.3f", pop());
+ printf("Error: Operator is missing!\n");
+ return EXIT_FAILURE;
+ }
+
+ if (stack_pointer != 1) {
+ printf("Error: invalid expression (stack has %u values).\n", stack_pointer);
+ return EXIT_FAILURE;
}
+ printf("result: %.3f\n", pop());
return EXIT_SUCCESS;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (op_count == 0 && argc > 2) { | |
| printf("Error: Operator is missing!"); | |
| } else { | |
| printf("result: %.3f", pop()); | |
| } | |
| return EXIT_SUCCESS; | |
| return EXIT_SUCCESS; | |
| if (op_count == 0 && argc > 2) { | |
| printf("Error: Operator is missing!\n"); | |
| return EXIT_FAILURE; | |
| } else { | |
| printf("result: %.3f", pop()); | |
| } | |
| return EXIT_SUCCESS; |
🤖 Prompt for AI Agents
In chapter_5/exercise_5_10/expr.c around lines 97 to 103, the missing-operator
branch currently only prints an error without newline and returns EXIT_SUCCESS
while still calling pop(), and it doesn't detect extra values left on the stack;
change the logic to validate that exactly one value remains on the stack after
processing (e.g., check stack_count == 1 or equivalent), if not print a clear
error message ending with a newline and return EXIT_FAILURE without calling
pop(), otherwise pop the single result, print it with a trailing newline and
return EXIT_SUCCESS.
|
@JebediahS Great to see you contributing to this PR! Before me taking a look, can you first resolve the merge conflicts and address all issues found by @coderabbitai? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 80244 @ohkimur, I've gathered the details about the merge conflicts and review issues. Here's a summary to help @JebediahS understand what needs to be addressed: 🔴 Merge ConflictsThe PR currently has merge conflicts that must be resolved first:
You'll need to: git fetch upstream
git merge upstream/main
# Resolve conflicts
git commit
git push🔍 CodeRabbit Review IssuesCodeRabbit found several categories of issues across the codebase. Here are the key issues specific to Critical Issues in
|
Filling the stack should start from element with index 0.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chapter_5/exercise_5_10/expr.c (1)
91-93: Add newline and validate stack state before printing result.Two issues:
- The result output lacks a trailing newline, inconsistent with error messages.
- No validation that exactly one value remains on the stack. If multiple values remain (e.g.,
./expr 5 1 2without an operator), only the top value is printed, silently ignoring the incomplete expression.Apply this diff to add validation and fix formatting:
+ // Validate exactly one result remains + if (stack_pointer == 0) { + printf("Error: no result (empty stack).\n"); + return EXIT_FAILURE; + } + if (stack_pointer > 1) { + printf("Error: incomplete expression (%u values remain on stack).\n", stack_pointer); + return EXIT_FAILURE; + } + - printf("result: %.3f", pop()); + printf("result: %.3f\n", pop()); return EXIT_SUCCESS;
♻️ Duplicate comments (1)
chapter_5/exercise_5_10/expr_test.sh (1)
4-7: Fix inverted error message.The condition
[ ! -f expr ]is true when the file does not exist, but line 5 prints "File 'expr' found!" which is the opposite. This contradicts the past review comment marked as "addressed."Apply this diff:
if [ ! -f expr ]; then - echo "File 'expr' found!" + echo "File 'expr' not found!" exit 1 fi
🧹 Nitpick comments (4)
chapter_5/exercise_5_10/expr.c (4)
98-98: Stack indexing fix looks correct; consider documenting the invariant.The pre-decrement in
popand post-increment inpushcorrectly implement 0-based indexing withstack_pointerpointing to the next free slot. This resolves the off-by-one bug described in the PR.Consider adding a brief comment documenting the stack pointer invariant:
+// Invariant: stack_pointer always points to the next free slot (0-based indexing) unsigned char stack_pointer = 0; float stack[STACK_SIZE];Also applies to: 107-107
20-23: Consider usingstrtoffor robust numeric parsing.The current approach with
atofandstrcmp(argv[i], "0") == 0has limitations:
atof("0.0")returns0, failing theif (number ...)check, then falls back to thestrcmpwhich only matches"0"exactly, causing valid inputs like"0.0"or"-0"to be rejected.atofaccepts malformed input like"123abc", silently truncating to123.Using
strtofwith end-pointer validation ensures only well-formed numeric strings are accepted.Apply this diff to use strtof with proper validation:
- float number = atof(argv[i]); + char *endptr; + float number = strtof(argv[i], &endptr); - if (number || strcmp(argv[i], "0") == 0) { + // Valid if conversion consumed chars and reached end (or whitespace) + if (endptr != argv[i] && (*endptr == '\0' || isspace(*endptr))) { push(number);
8-8: Consider usingsize_tforstack_pointer.Using
unsigned charlimits the stack pointer to 255, which is sufficient forSTACK_SIZEof 15 but not idiomatic.size_tis the standard type for array indices and sizes, improving clarity and consistency with comparisons againstSTACK_SIZE.Apply this diff:
-unsigned char stack_pointer = 0; +size_t stack_pointer = 0;You'll also need to update format specifiers in error messages from
%uto%zuif you add any that printstack_pointer.
96-103: Returning 0 frompopon error is ambiguous.If the stack is empty,
pop()returns0, which is indistinguishable from a valid0.0value popped from the stack. This makes error detection unreliable for callers.Consider returning status via an out-parameter or using a dedicated error-signaling mechanism (e.g., return
bool/intsuccess status with value via pointer). However, given the current usage inmain(line 91) where errors are already handled before callingpop, and the fact that this is K&R-style educational code, this refactor is optional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
chapter_5/exercise_5_10/expr.c(2 hunks)chapter_5/exercise_5_10/expr_test.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
chapter_5/exercise_5_10/expr_test.sh
[warning] 79-79: 'expr' expects 3+ arguments, but sees 2. Make sure each operator/operand is a separate argument, and escape <>&|.
(SC2307)
🔇 Additional comments (2)
chapter_5/exercise_5_10/expr_test.sh (2)
77-85: Shellcheck warning is a false positive.The static analysis warning on line 79 (
'expr' expects 3+ arguments, but sees 2) is intentional—this test deliberately provides insufficient arguments to verify the missing-operator error case.This is not an issue; the warning can be safely ignored or suppressed with a shellcheck directive if desired:
# shellcheck disable=SC2307 result=$(./expr 5 1)
14-87: Good test coverage.The test suite covers key scenarios effectively:
- Basic arithmetic operations (addition, subtraction)
- Commutativity verification
- Complex RPN expression with helpful inline trace
- Edge case error handling (division by zero, missing operator)
The inline step-by-step comments for Test 4 (lines 48-59) are particularly helpful for understanding the expected evaluation order.
| result=$(./expr 5 1) | ||
| expected="Error: Operator is missing!" | ||
| if [[ "$result" == "$expected" ]]; then | ||
| echo "Test $T PASS: Missing operator" | ||
| else | ||
| echo "Test $T FAIL: Expected '$expected', got '$result'" | ||
| fi |
There was a problem hiding this comment.
Test 6 expected output is inconsistent with error message formatting.
The test expects "Error: Operator is missing!" without a trailing newline. While this matches the current behavior of expr.c (line 91), it's inconsistent with:
- Other error messages in
expr.cwhich include\n - The general convention that command-line tools print newlines after output
This is a minor cosmetic issue, but fixing it in both expr.c and this test would improve consistency.
Related: The suggested fix in the review comment for expr.c lines 91-93 addresses this by adding proper validation and a newline to the result output.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 79-79: 'expr' expects 3+ arguments, but sees 2. Make sure each operator/operand is a separate argument, and escape <>&|.
(SC2307)
🤖 Prompt for AI Agents
In chapter_5/exercise_5_10/expr_test.sh around lines 79 to 85, the test expects
the error string "Error: Operator is missing!" without a trailing newline which
is inconsistent with other error messages and the intended fix in expr.c; update
the test to expect a newline-terminated string (e.g., "Error: Operator is
missing!\n") or use a comparison that ignores trailing newlines (for example,
compare trimmed output) so the test matches the corrected expr.c behavior that
prints the error message followed by a newline.
2 participants
Filling the stack should start from element with index 0.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.