Implemented 16-bit version of the code for more flexibility

[kkaname]

I have implemented much simpler version and also 32-bit for more code flexibility

Summary by CodeRabbit

• New Features
  • Added binary representation display to visualize integer values in binary format.
  • Updated rotation algorithm to work with smaller integer data types and simplified rotation count handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

A single exercise file is refactored to change the rightrot function from operating on unsigned int to unsigned short, with a new to_binary helper function introduced for binary representation printing. The include directive is updated from math.h to limits.h, and the main function is restructured to use the revised functions.

Changes

Cohort / File(s)	Summary
Right rotation refactoring chapter_2/exercise_2_08/rightrot.c	Function signature updated: rightrot changed from unsigned int parameters to unsigned short x, int n. New to_binary(unsigned short n) helper function added for binary representation printing. Previous binary printing and right-rotation logic for unsigned int removed. Includes changed from math.h to limits.h. Main function updated to demonstrate usage of refactored functions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5–10 minutes

• Area of focus: Verify correctness of the bitwise operation (x >> n) | (x << (w - n)) where w is the bit width of unsigned short and proper modulo handling of n.
• Consideration: Ensure the to_binary helper correctly handles the unsigned short type width and the zero-rotation edge case.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	⚠️ Warning	The title mentions '16-bit version' but the PR objectives describe implementing a '32-bit variant', and the actual changes show conversion from unsigned int to unsigned short (16-bit). There is a discrepancy between the title and stated objectives.	Align the PR title with the actual changes: either correct it to reflect the 16-bit implementation or update to match the stated 32-bit objective if that was the intended goal.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

chapter_2/exercise_2_08/rightrot.c (1)

10-10: Consider reducing the array size.

The binary array is sized at 32, but unsigned short typically uses 16 bits. While this is safe, it wastes stack space.

Apply this diff to optimize the array size:

-    int binary[32];
+    int binary[sizeof(unsigned short) * CHAR_BIT];

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bdcee72 and 699c5fd.

📒 Files selected for processing (1)

• chapter_2/exercise_2_08/rightrot.c (1 hunks)

🧰 Additional context used

🪛 Cppcheck (2.18.0)

chapter_2/exercise_2_08/rightrot.c

[warning] 23-23: Uninitialized variable

(uninitvar)

🔇 Additional comments (2)

chapter_2/exercise_2_08/rightrot.c (2)

1-1: LGTM!

The change from math.h to limits.h is correct, as CHAR_BIT is defined in limits.h.

---

22-24: False positive from static analysis.

The static analysis warning about an uninitialized variable at line 23 is a false positive. The preceding while loop (lines 18-21) initializes all elements of binary from index 0 to i-1, and the for loop only accesses those initialized indices.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Undefined behavior when n is 0.

When n is 0 (or a multiple of w), line 32 evaluates to (x >> 0) | (x << w). Shifting by the bit width or more is undefined behavior in C.

Apply this diff to fix the issue:

 unsigned short rightrot(unsigned short x, int n) {
     unsigned w = sizeof(unsigned short) * CHAR_BIT;
     n %= w;
-
+    if (n == 0)
+        return x;
     return (x >> n) | (x << (w - n));
 }

🤖 Prompt for AI Agents

In chapter_2/exercise_2_08/rightrot.c around lines 28 to 33, the code does a
left shift by (w - n) which becomes a shift by w when n is 0 (or a multiple of
w) causing undefined behavior; fix by after computing n %= w, check if n == 0
and simply return x to avoid any shift by w, otherwise perform the existing
rotate expression.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use correct format specifier for unsigned short.

The format specifier %d is for signed integers. For unsigned short, use %hu or cast to unsigned and use %u.

Apply this diff:

-    printf("x = %d -->", x);
+    printf("x = %hu -->", x);

📝 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.

Suggested change

	printf("x = %d -->", x);
	printf("x = %hu -->", x);

🤖 Prompt for AI Agents

In chapter_2/exercise_2_08/rightrot.c around line 39, the printf uses the signed
integer specifier "%d" for an unsigned short variable x; change the format
specifier to "%hu" (or cast x to unsigned and use "%u") so the value prints
correctly, and update the printf call to use the chosen unsigned specifier.

[ohkimur]

@kkaname Great to see you contributing again! Can you ensure first the @coderabbitai issues are addressed first before I take a look?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get all review comments from PR #99
gh pr view 99 --json reviewThreads --jq '.reviewThreads[] | {author: .comments[0].author.login, body: .comments[0].body, path: .path, line: .line}'

Length of output: 674

---

🏁 Script executed:

#!/bin/bash
# Also get the general PR comments to check for any bot comments there
gh pr view 99 --json comments --jq '.comments[] | {author: .author.login, body: .body, createdAt: .createdAt}'

Length of output: 9828

---

🏁 Script executed:

#!/bin/bash
# Get PR review summary
gh pr view 99 --json reviews --jq '.reviews[] | {author: .author.login, state: .state, body: .body}'

Length of output: 2600

---

🏁 Script executed:

#!/bin/bash
# Get all review comments on files in PR #99
gh api repos/ohkimur/the-c-programming-language-2nd-edition-solutions/pulls/99/comments --jq '.[] | {author: .user.login, body: .body, path: .path, line: .line, created_at: .created_at}'

Length of output: 2797

---

@ohkimur Here's a summary of the issues raised by CodeRabbit that need to be addressed:

Pre-merge Check Warnings

1. Title Check (⚠️ Warning): The PR title claims to implement a "32-bit version," but the changes actually refactor to use unsigned short (16-bit) instead of unsigned int (32-bit), which contradicts the stated objective. The title should be updated to accurately reflect the changes.

2. Docstring Coverage (⚠️ Warning): Docstring coverage is 33.33%, which is below the required threshold of 80%.

Actionable Code Issues

1. 🔴 Critical - Undefined Behavior (Line 33): When n is 0 (or a multiple of w), the expression (x >> n) | (x << (w - n)) causes a left shift by w, which is undefined behavior in C. The fix is to add a check: if n == 0, return x directly.

2. 🟡 Minor - Wrong Format Specifier (Line 39): The printf uses %d (for signed integers) to print an unsigned short variable. It should use %hu instead.

Nitpick

3. Array Size Optimization (Line 10): The binary array is sized at 32, but unsigned short typically uses 16 bits. While safe, it could be optimized to sizeof(unsigned short) * CHAR_BIT.

The most critical issue is #1 (undefined behavior), which must be fixed. Issues #2 and the title mismatch should also be addressed before merge.

[ohkimur]

@cooderabbitai I thought I disabled Docstring Coverage for this project. Can you check your configuration please?

[kkaname]

@ohkimur ok its my mistake that i put 32 bit instead of saying it to be 16 since i used unsigned short type.
so should i correct this issue and put a new commit?