-
-
Notifications
You must be signed in to change notification settings - Fork 140
Fix exercise 1-18 #92
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
Conversation
WalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
chapter_1/exercise_1_18/trailing_blanks.c (1)
15-20: Logic is correct; consider minor readability improvement.The implementation correctly captures the new length and skips printing blank lines (lines reduced to just
"\n"). The conditionlen > 1 || line[0] != '\n'works correctly but could be made more explicit.Consider this equivalent but more explicit condition:
- if (len > 1 || line[0] != '\n') + if (!(len == 1 && line[0] == '\n')) { printf("%s", line); }This makes it clearer that you're specifically filtering out lines that are exactly a single newline character.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chapter_1/exercise_1_18/trailing_blanks.c(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
chapter_1/exercise_1_18/trailing_blanks.c (1)
chapter_1/exercise_1_19/reverse.c (1)
length(49-57)
🔇 Additional comments (3)
chapter_1/exercise_1_18/trailing_blanks.c (3)
6-6: LGTM! Function signature correctly updated.The return type change from
voidtointallows the function to communicate the new string length after processing, which is essential for the correct implementation of blank line deletion.
46-47: LGTM! Macros improve code readability.The
HAS_NEWLINEandNO_NEWLINEmacros make the intent clear and are appropriate for K&R C style. This is preferable to using magic numbers directly in the code.
49-72: Excellent fix! All PR objectives successfully addressed.The implementation correctly handles both issues mentioned in the PR:
✓ EOF without newline: Starting at
i = length - 1instead oflength - 2correctly processes strings that don't end with'\n', fixing cases like"qwe"and"qwe \t "at EOF.✓ Blank line deletion: Lines that contain only whitespace (spaces, tabs, newlines) are now reduced to
"\n"(or empty string if no newline was present) and filtered appropriately by the main function.The logic is sound:
- Loop strips all trailing whitespace (including newline)
- After the loop,
ipoints to the last non-whitespace character (or -1 if all whitespace)- Newline is preserved when originally present
- String is properly null-terminated in both code paths
Edge cases are handled correctly:
- All whitespace with newline:
" \t\n"→"\n"(suppressed)- All whitespace without newline:
" \t"→""(printed, but acceptable since no newline means it's not a complete line)- Mixed content:
"abc \t\n"→"abc\n"- EOF cases:
"abc"→"abc"
Let me remind you of the original text of the exercise:
Resolved issues:
The
remove_trailing_blanksfunction was not working correctly.Its task was to remove trailing blanks and tabs from the passed string, but the string does not always end with a newline character (
'\n'). If it is the end of the file, it may end withEOF, i.e."qweEOF", in which case there is no need to add the'\n'character.Also, in the
remove_trailing_blankslogic, we initially started iterating through characters withi = length - 2, which again would behave incorrectly in the case ofEOF.Try this input (EOF means, for example, Ctrl+D in the terminal):
"qweEOF"(the last character will be removed)"qwe \t EOF"('\n'will be added, which was not in the original string).The logic for "delete entirely blank lines" was not implemented.
Try this input:
"\n""\t\t\t\n"Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.