-
-
Notifications
You must be signed in to change notification settings - Fork 140
Implemented the loop for reading input into a string without using th… #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…e relational operators. As per asked in the excercise question, I have implemented the loop using while loop and break statement without using any relational operator(&& and ||).
WalkthroughReplaced guarded while loop with an infinite Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
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: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
chapter_2/exercise_2_02/loop.c (1)
9-13: Optional: Consider removing commented-out code.The commented code from the previous implementation provides useful context but could be removed for cleaner code. The NOTE at lines 40-45 already explains the multiplication approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chapter_2/exercise_2_02/loop.c(1 hunks)
🔇 Additional comments (1)
chapter_2/exercise_2_02/loop.c (1)
15-31: Implementation successfully avoids relational operators, but needs fixes.The while(1) loop with separate break conditions successfully avoids using
&&and||operators as required by the exercise. The buffer overflow protection logic (lines 16-18) is correct.However, the critical bug on line 20 must be fixed before this implementation is correct.
| else if (c == EOF){ | ||
| printf("\n"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questionable: Extra newline printed on EOF.
Lines 25-28 print a newline when EOF is encountered, but not for other exit conditions (newline or buffer full). This creates inconsistent output behavior.
Consider whether this extra newline is necessary or if EOF should be handled like the newline case (just break).
🤖 Prompt for AI Agents
In chapter_2/exercise_2_02/loop.c around lines 25 to 28, the code prints an
extra newline when c == EOF which causes inconsistent output compared to other
exit conditions; remove the printf("\n") so EOF is handled the same as the
newline/buffer-full cases (just break), or restructure the branch to break
immediately on EOF without emitting any characters.
|
@ohkimur So i have implemented the new changes, and as per you said earlier you can merge the file. |
|
Awesome job @kkaname! The PR is almost ready to merge. A couple of nitpicks for the consistency of the repo. Let’s define the variables one per line explicitly as you saw in my code. Also adress the finding be @coderabbitai and we’re good to go. NOTE: in case you want to contribute more it’ll be amazing if we enforce single line variable definition in the linting/formatting so other contributors know about this in advance. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11766 @ohkimur Thank you for the clarification! Just to note, my previous finding was actually positive - I confirmed that the critical EOF detection issue was fixed and the code is working correctly now. The only remaining item for consistency is the variable declaration style. Currently on line 7: int c, i = 0;Should be split into: int c;
int i = 0;This matches the repository's coding style of defining variables one per line. @kkaname - this is a simple change needed for consistency with the rest of the codebase. Just split the variable declarations on line 7 into separate lines as shown above. Regarding the linting/formatting note: For C projects, tools like |
@coderabbitai Maybe you can take care of the single line variable definition and the finding you found so we make @kkaname life easier. You can update the PR and I’ll review the changes. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1030 @ohkimur I appreciate the trust, but I'm unable to commit code directly to the repository. However, I can make this very easy for either you or @kkaname! The change needed is simple - just one line needs to be modified: Current code (line 7): int c, i = 0;Should be changed to: int c;
int i = 0;That's the only change needed for the variable declaration style consistency. To clarify my previous finding: I was actually confirming that the code is working correctly - the EOF handling issue has been properly fixed. So there's no functional issue to address, just this style consistency change. Options to apply this change:
Either way, this is the final change needed before merge! 🐰 |
Refactor variable declarations for clarity.
|
@ohkimur Done!
It was a great to contribute to this repository as this is my first ever contribution to public repo, Thanks a lot. |
…e relational operators.
As per asked in the excercise question, I have implemented the loop using while loop and break statement without using any relational operator(&& and ||).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.