Add producer-consumer example - #167
Conversation
📝 WalkthroughWalkthroughThe PR updates the livelock-cycle CLI option and adds a producer-consumer example with custom semaphore synchronization, a circular buffer harness, build tooling, and debugging instructions. ChangesCLI option update
Producer-consumer example
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/producer-consumer/Makefile (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare command targets as phony.
If a file or directory named
runorcleanexists, Make may skip these recipes. Add.PHONYdeclarations for all command targets.+.PHONY: run gdb vi emacs clean dist + run: $(FILE)Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/producer-consumer/Makefile` at line 9, Declare the Makefile command targets, including run and clean, as phony using a .PHONY declaration so their recipes always execute even when matching files or directories exist.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/launch.c`:
- Around line 120-122: Update the inline -lc option handling near
ENV_MAX_LIVELOCK_CYCLE_LIMIT to validate the entire numeric suffix, not just its
first character, before calling setenv. Reuse the validator used by the
separated-form argument when available, and reject values such as -lc12x rather
than storing them.
In `@test/producer-consumer/Makefile`:
- Around line 12-14: Update the $(FILE) target prerequisites in the Makefile to
include partial-solution.c alongside $(FILE).c, while leaving the existing
compilation recipe unchanged.
- Line 9: Update the Makefile’s run target so it executes the built $(FILE)
binary after ensuring $(FILE) is built, rather than only declaring it as a
prerequisite. Preserve the existing build dependency and use the project’s
established executable invocation convention if available.
In `@test/producer-consumer/partial-solution.c`:
- Around line 22-53: Rename the custom semaphore API from sem_* to mysem_* so
McMini recognizes the intended implementation. In
test/producer-consumer/partial-solution.c:22-53, rename sem_t and
sem_init/sem_wait/sem_post; update the corresponding type declarations and
prototypes in test/producer-consumer/producer-consumer.c:36-47, initialization
at :97-98, producer calls at :122-127, and consumer calls at :136-141. Correct
the API statement in test/producer-consumer/000-README:5-8 to match the renamed
interface.
- Around line 29-33: Update the third parameter of sem_init in
partial-solution.c from int to unsigned int so its definition matches the
producer-consumer.c declaration and preserves the expected ABI contract when
initializing sem->count.
---
Nitpick comments:
In `@test/producer-consumer/Makefile`:
- Line 9: Declare the Makefile command targets, including run and clean, as
phony using a .PHONY declaration so their recipes always execute even when
matching files or directories exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8a21404-83c6-4d23-9715-bd7434bc97ad
📒 Files selected for processing (5)
src/launch.ctest/producer-consumer/000-READMEtest/producer-consumer/Makefiletest/producer-consumer/partial-solution.ctest/producer-consumer/producer-consumer.c
| else if (cur_arg[0][1] == 'l' && cur_arg[0][2] == 'c' && | ||
| isdigit(cur_arg[0][3])) { | ||
| setenv(ENV_MAX_LIVELOCK_CYCLE_LIMIT, cur_arg[0] + 3, 1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed -lc<num> values instead of silently truncating them.
The branch validates only the first suffix character, so -lc12x stores 12x; downstream strtoul(..., 10) then consumes the 12 prefix and silently applies the wrong limit. Validate that the entire suffix is numeric before calling setenv, ideally sharing the same validator with the separated form.
Proposed validation
else if (cur_arg[0][1] == 'l' && cur_arg[0][2] == 'c' &&
isdigit(cur_arg[0][3])) {
+ char *endptr;
+ strtol(cur_arg[0] + 3, &endptr, 10);
+ if (endptr[0] != '\0') {
+ fprintf(stderr, "%s: illegal value\n",
+ "--max-livelock-cycle-limit");
+ exit(1);
+ }
setenv(ENV_MAX_LIVELOCK_CYCLE_LIMIT, cur_arg[0] + 3, 1);
cur_arg++;
}📝 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.
| else if (cur_arg[0][1] == 'l' && cur_arg[0][2] == 'c' && | |
| isdigit(cur_arg[0][3])) { | |
| setenv(ENV_MAX_LIVELOCK_CYCLE_LIMIT, cur_arg[0] + 3, 1); | |
| else if (cur_arg[0][1] == 'l' && cur_arg[0][2] == 'c' && | |
| isdigit(cur_arg[0][3])) { | |
| char *endptr; | |
| strtol(cur_arg[0] + 3, &endptr, 10); | |
| if (endptr[0] != '\0') { | |
| fprintf(stderr, "%s: illegal value\n", | |
| "--max-livelock-cycle-limit"); | |
| exit(1); | |
| } | |
| setenv(ENV_MAX_LIVELOCK_CYCLE_LIMIT, cur_arg[0] + 3, 1); | |
| cur_arg++; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/launch.c` around lines 120 - 122, Update the inline -lc option handling
near ENV_MAX_LIVELOCK_CYCLE_LIMIT to validate the entire numeric suffix, not
just its first character, before calling setenv. Reuse the validator used by the
separated-form argument when available, and reject values such as -lc12x rather
than storing them.
| CFLAGS += -g3 -O0 | ||
| LDLIBS += -lpthread | ||
|
|
||
| run: $(FILE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make run actually execute the example.
run only builds $(FILE); make run never launches the producer-consumer program.
Proposed fix
run: $(FILE)
+ ./$(FILE)📝 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.
| run: $(FILE) | |
| run: $(FILE) | |
| ./$(FILE) |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 9-9: Target "run" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/producer-consumer/Makefile` at line 9, Update the Makefile’s run target
so it executes the built $(FILE) binary after ensuring $(FILE) is built, rather
than only declaring it as a prerequisite. Preserve the existing build dependency
and use the project’s established executable invocation convention if available.
| $(FILE): $(FILE).c | ||
| $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ \ | ||
| $(FILE).c partial-solution.c $(LDLIBS) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Track partial-solution.c as a build prerequisite.
The recipe compiles partial-solution.c, but the target only depends on producer-consumer.c. Changes to the semaphore implementation therefore leave the executable stale until a manual clean or forced rebuild.
Proposed fix
-$(FILE): $(FILE).c
+$(FILE): $(FILE).c partial-solution.c📝 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.
| $(FILE): $(FILE).c | |
| $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ \ | |
| $(FILE).c partial-solution.c $(LDLIBS) | |
| $(FILE): $(FILE).c partial-solution.c | |
| $(CC) $(CPPFLAGS) $(CFLAGS) -o $@ \ | |
| $(FILE).c partial-solution.c $(LDLIBS) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/producer-consumer/Makefile` around lines 12 - 14, Update the $(FILE)
target prerequisites in the Makefile to include partial-solution.c alongside
$(FILE).c, while leaving the existing compilation recipe unchanged.
| struct sem { | ||
| int count; | ||
| pthread_mutex_t mutex; | ||
| }; | ||
| typedef struct sem sem_t; | ||
| // Unlike Java, C/C++ require a semicolon to end the 'struct sem' statement. | ||
|
|
||
| int sem_init(sem_t *sem, int ignore, int init) { | ||
| sem->count = init; | ||
| // Anybody who wants to use sem->count should acquire this lock first. | ||
| pthread_mutex_init(&(sem->mutex), NULL); // See 'man pthread_mutex_init' | ||
| } | ||
|
|
||
| int sem_post(sem_t *sem) { | ||
| pthread_mutex_lock( &(sem->mutex) ); | ||
| sem->count += 1; | ||
| pthread_mutex_unlock( &(sem->mutex) ); | ||
| } | ||
|
|
||
| void block(sem_t *sem); // Used by sem_wait() | ||
|
|
||
| int sem_wait(sem_t *sem) { | ||
| pthread_mutex_lock( &(sem->mutex) ); | ||
| sem->count -= 1; | ||
| if (sem->count < 0) { | ||
| // We must release lock, or no one can ever call sem_post() for us. | ||
| pthread_mutex_unlock( &(sem->mutex) ); | ||
| block(sem); | ||
| pthread_mutex_lock( &(sem->mutex) ); | ||
| } | ||
| pthread_mutex_unlock( &(sem->mutex) ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rename the custom semaphore API before running under McMini.
The README says McMini requires mysem_*, but the implementation and harness still use sem_*; McMini will model the custom calls as real semaphore operations rather than expose the intended blocking bug.
test/producer-consumer/partial-solution.c#L22-L53: renamesem_tandsem_init/sem_wait/sem_postto a custommysem_*API.test/producer-consumer/producer-consumer.c#L36-L47: update the type and prototypes to the renamed API.test/producer-consumer/producer-consumer.c#L97-L98: initialize the renamed semaphores.test/producer-consumer/producer-consumer.c#L122-L127: update producer wait/post calls.test/producer-consumer/producer-consumer.c#L136-L141: update consumer wait/post calls.test/producer-consumer/000-README#L5-L8: correct the statement once the rename is applied.
📍 Affects 3 files
test/producer-consumer/partial-solution.c#L22-L53(this comment)test/producer-consumer/producer-consumer.c#L36-L47test/producer-consumer/producer-consumer.c#L97-L98test/producer-consumer/producer-consumer.c#L122-L127test/producer-consumer/producer-consumer.c#L136-L141test/producer-consumer/000-README#L5-L8
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/producer-consumer/partial-solution.c` around lines 22 - 53, Rename the
custom semaphore API from sem_* to mysem_* so McMini recognizes the intended
implementation. In test/producer-consumer/partial-solution.c:22-53, rename sem_t
and sem_init/sem_wait/sem_post; update the corresponding type declarations and
prototypes in test/producer-consumer/producer-consumer.c:36-47, initialization
at :97-98, producer calls at :122-127, and consumer calls at :136-141. Correct
the API statement in test/producer-consumer/000-README:5-8 to match the renamed
interface.
| int sem_init(sem_t *sem, int ignore, int init) { | ||
| sem->count = init; | ||
| // Anybody who wants to use sem->count should acquire this lock first. | ||
| pthread_mutex_init(&(sem->mutex), NULL); // See 'man pthread_mutex_init' | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -nP '^\s*int\s+sem_init\s*\(' \
test/producer-consumer/partial-solution.c \
test/producer-consumer/producer-consumer.cRepository: mcminickpt/mcmini
Length of output: 354
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- partial-solution.c ---\n'
cat -n test/producer-consumer/p_partial-solution.c 2>/dev/null || cat -n test/producer-consumer/partial-solution.c | sed -n '1,80p'
printf '\n--- producer-consumer.c relevant lines ---\n'
cat -n test/producer-consumer/producer-consumer.c | sed -n '1,80p'
printf '\n--- header references ---\n'
rg -n 'sem_init|sem_t' test/producer-consumer -C 2Repository: mcminickpt/mcmini
Length of output: 246
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- partial-solution.c ---'
cat -n test/producer-consumer/partial-solution.c | sed -n '1,80p'
printf '%s\n' ''
printf '%s\n' '--- producer-consumer.c relevant lines ---'
cat -n test/producer-consumer/producer-consumer.c | sed -n '1,80p'
printf '%s\n' ''
printf '%s\n' '--- header references ---'
rg -n 'sem_init|sem_t' test/producer-consumer -C 2Repository: mcminickpt/mcmini
Length of output: 12096
Fix sem_init()’s third parameter type.
producer-consumer.c declares the third argument as unsigned int value, but partial-solution.c defines it as int init. The definition must match the declaration from the same ABI contract, so sem->count is not incorrectly initialized by values outside the range expected by the caller.
Proposed fix
-int sem_init(sem_t *sem, int ignore, int init) {
+int sem_init(sem_t *sem, int ignore, unsigned int init) {📝 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.
| int sem_init(sem_t *sem, int ignore, int init) { | |
| sem->count = init; | |
| // Anybody who wants to use sem->count should acquire this lock first. | |
| pthread_mutex_init(&(sem->mutex), NULL); // See 'man pthread_mutex_init' | |
| } | |
| int sem_init(sem_t *sem, int ignore, unsigned int init) { | |
| sem->count = init; | |
| // Anybody who wants to use sem->count should acquire this lock first. | |
| pthread_mutex_init(&(sem->mutex), NULL); // See 'man pthread_mutex_init' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/producer-consumer/partial-solution.c` around lines 29 - 33, Update the
third parameter of sem_init in partial-solution.c from int to unsigned int so
its definition matches the producer-consumer.c declaration and preserves the
expected ABI contract when initializing sem->count.
Summary by CodeRabbit
New Features
Documentation
-lc<num>option format.