opensifli/sifli-sdk/sifli-code-review

SiFli SDK Code Review

Comprehensive code review for SiFli SDK projects covering general code, example projects, and documentation.

Trigger

When the user says "review下代码", "审阅下代码", "审查下代码", "review this code", "从gerrit拉取", "审查gerrit", "review这个commit", "审查这个提交", "审阅这个sha", or similar phrases, use this skill to perform a structured review of the current changes, a local commit, or a Gerrit commit.

Step 0: Determine the Review Target

Ask the user to clarify whether the review is for:

1. Local changes — uncommitted working tree changes (default if user just says "review下代码")
2. Local commit SHA — a commit that already exists in the local repository. The user provides a SHA hash (full or short, e.g. a1b2c3d or 028f3a59f).
3. Gerrit commit — a specific commit on Gerrit. The user must provide one of:

• A Gerrit change URL, e.g. http://10.21.10.179:8080/c/SiFli-SDK/+/12345 or ssh://10.21.10.179:29418/SiFli-SDK
• A Gerrit change number, e.g. 12345
• A refs/changes/... refspec
• A commit SHA (40-char hash)

How to distinguish local SHA vs Gerrit SHA: If the user provides a SHA hash, first check if it exists locally with git cat-file -t <sha>. If it exists locally, treat it as a local commit SHA (option 2). If it does not exist locally, treat it as a Gerrit SHA and attempt to fetch it from Gerrit (option 3).

Step 0a: Review Local Commit SHA

When the user provides a local SHA to review, use these commands:

Step 1 — Verify the commit exists locally:

git cat-file -t <sha>

Expected output: commit. If the command fails, the SHA doesn't exist locally — fall through to the Gerrit fetch flow (Step 0b).

Step 2 — Determine the diff base:

The diff shows what the commit introduced. Use the commit's first parent:

git diff <sha>~1..<sha>

If the commit is a merge commit (multiple parents), you may want to use a different base:

# For merge commits, diff against the first parent
git diff <sha>~1..<sha>
# Or diff against a specific branch
git diff <branch>..<sha>

Step 3 — Check out the commit to read files:

git checkout <sha>

IMPORTANT — STAY on the reviewed commit after the review (do NOT revert): Checking out a detached HEAD moves away from the current branch, but keep the checkout once the review is done — do NOT `git checkout -` back to the original branch. The user wants to browse the code against the report, so the working tree must remain at the reviewed commit so the line numbers quoted in the report match the code on disk. Record the original branch name (git branch --show-current) before checking out, and only switch back later if the user explicitly asks (git checkout <original-branch>).

Step 4 — Read the commit message:

git log -1 --format=full <sha>

Step 5 — Show the diff for analysis:

git diff <sha>~1..<sha>
git diff --name-only <sha>~1..<sha>

Step 0b: Fetch from Gerrit

The Gerrit server is at ssh://10.21.10.179:29418/SiFli-SDK. To fetch a change:

If given a Gerrit URL like http://10.21.10.179:8080/c/SiFli-SDK/+/12345 or http://10.21.10.179:8080/c/SiFli-SDK/+/12345/5:

• Extract the change number (e.g. 12345) and optional patchset number (e.g. 5)
• The last 2 digits of the change number determine the ref directory: refs/changes/<last2>/<change>/<patchset>
• Example: change 12345 → refs/changes/45/12345/

Step 1 — Fetch the change:

If a specific patchset is given:

git fetch origin refs/changes/<last2>/<change_number>/<patchset>

If no patchset is given, fetch all patchsets for that change:

git fetch origin refs/changes/<last2>/<change_number>/*

Then find the latest patchset from the fetched refs and checkout:

git checkout FETCH_HEAD

If given a commit SHA, try fetching it directly:

git fetch origin <sha>
git checkout FETCH_HEAD

Step 2 — Determine the diff base:

After fetching and checking out the commit, determine what to diff against. Use the actual parent commit (FETCH_HEAD's first parent):

git diff FETCH_HEAD~1..FETCH_HEAD

IMPORTANT — Use `FETCH_HEAD~1`, not `HEAD~1`: After git fetch (without checkout), FETCH_HEAD points to the fetched commit. HEAD is your current branch, which is unrelated. Using HEAD~1..FETCH_HEAD will give you the diff between your branch and the fetched commit — NOT the actual changes in the commit!

After git fetch && git checkout FETCH_HEAD, HEAD now equals FETCH_HEAD, so HEAD~1..HEAD works. But git diff FETCH_HEAD~1..FETCH_HEAD is always safe regardless of checkout state.

Or if the user specifies a branch to compare against:

git diff <branch>..HEAD

Also show the commit message (always reference FETCH_HEAD explicitly):

git log -1 --format=full FETCH_HEAD

IMPORTANT — Always use `FETCH_HEAD` in `git log`: After git fetch, HEAD is still your original branch. Running git log -1 without FETCH_HEAD will show the wrong commit message. Even after git checkout FETCH_HEAD, using FETCH_HEAD is safer — there's no ambiguity about what you're reading.

IMPORTANT — STAY on the reviewed commit after the review (do NOT revert): Same rule as Step 0a — after git checkout FETCH_HEAD and completing the review, remain on the reviewed commit so the user can keep browsing the code against the report. Do not git checkout - back to the original branch unless the user explicitly asks.

Step 0c: Review Local Changes (default)

If no Gerrit reference or local SHA is provided, review the current working tree.

First, identify what files are changed:

git diff --name-only

Also check for untracked files:

git status -s

Step 1: Classify Changed Files

Classify each changed file into one of four categories:

• General code: C source/header files, build scripts (SConscript, SConstruct, Kconfig), configuration files
• Example README: README.md or README_EN.md files in an example directory
• Example code: C source/header files in example/ directories (which also requires the general code checks)
• SDK documentation: files under docs/ directory (.md, .rst)

CRITICAL — Read source files yourself; do NOT delegate to sub-agents.

Sub-agents may lack access to FETCH_HEAD or the working tree. If they silently fail, the review proceeds with no actual code reading.

1. For Gerrit commits: git checkout FETCH_HEAD, then read files directly from disk. For local SHA commits: git checkout <sha>, then read files directly from disk. This also gives accurate line numbers.

2. For new files or files with >100 lines changed, read the FULL source file — not just the diff. Diffs lack surrounding context (control flow, callers, variable scope).

3. Read every line of substantial files (>50 lines changed). Do not skim — trace control flow through each function, check every condition and error path.

IMPORTANT — For new chip support changes, review must verify "the entire driver works correctly on the new chip", not just "the few lines changed are free of typos".

When the diff adds #if defined(NEW_CHIP) / #if !defined(NEW_CHIP) guards to add support for a new chip family, the diff itself only shows where the guards are placed. It does not show the execution flow for the new chip. You must reconstruct it mentally:

1. Traverse all execution paths for the new chip: For every function touched by the diff, trace through the function with the new chip's macros enabled, and verify each path is semantically correct. Do not assume that "skipping the old code" is sufficient — the new chip must still produce correct behavior (timeout values, register configurations, interrupt setup, etc.).

2. Contrast key parameters between old and new paths: Compare critical values (timeout durations, reload values, clock frequencies, interrupt windows) between the existing chip path and the new chip path. If they differ, ask whether the difference is intentional (hardware difference) or accidental (bug). For example: if the existing chip sets Reload2 to 50 seconds but the new chip path sets it to 5 seconds, flag it for confirmation.

3. Check macro naming consistency: Newly introduced condition macros (e.g. MY_CHIP) must be checked against existing macros in the same file (e.g. SOC_MY_CHIP). Inconsistent naming leads to silent compilation logic errors where the guard never activates.

4. Evaluate un-guarded code paths: Code paths that remain unchanged (not wrapped in any #if) may still be wrong for the new chip. Check whether functions called from modified code access registers or hardware blocks that don't exist on the new chip.

5. Check arithmetic consistency: When the diff involves expressions with mixed operators (e.g. freq * time + offset vs freq * (time + offset)), verify operator precedence produces the intended result. These bugs are invisible on a quick read.

For local changes:

git diff --name-only && git status -s   # identify files
git diff                                # read the diff

For local commit SHA:

git diff <sha>~1..<sha>                 # read the diff
git diff --name-only <sha>~1..<sha>     # identify changed files

Then checkout the commit (git checkout <sha>) and read each changed source file directly from disk with the Read tool.

For Gerrit changes:

git fetch origin refs/changes/<last2>/<change_number>/<patchset>
git checkout FETCH_HEAD
git diff HEAD~1..HEAD                   # read the diff

Then read each changed source file directly from disk with the Read tool.

Step 2: General Code Review (applies to all code files)

For each code file changed, check the following items:

2.1 Coding Style Compliance

Check against the project's .clang-format (Allman brace style, 4-space indent) and docs/source/en/contribute/coding_style.md:

• Braces: Allman style — braces on their own line, not at end of statement. Exception: switch-case aligns case with switch.
• Indentation: 4 spaces. Preprocessor directives indented with BeforeHash style.
• Naming:
• Variables: lowercase with _ (snake_case). Global variables prefixed with module name. Static variables preferably with s_ prefix.
• Functions: lowercase with _. Internal static functions prefixed with _.
• Macros/Enum values: UPPERCASE with _.
• Types: struct/enum name + _t suffix.
• Spacing: Space before ( in if, for, while, switch. No spaces inside parentheses of expressions. Operators have spaces around them.
• Header guards: #ifndef __FILE_H__ / #define __FILE_H__ format.
• Comments: Must use English (not Chinese). Function comments use doxygen style (/** ... */). Statement comments above or to the right only.
• SortIncludes: false — includes should be preserved as-is, do not reorder.
• No consecutive blank lines: At most one blank line unless special need.

2.2 Logic Correctness & Boundary Conditions

• Are all error paths handled? Check return values of functions that can fail.
• Are array indices bounds-checked?
• Are pointers checked for NULL before dereference?
• Are buffer sizes validated to prevent overflow?
• Are loop termination conditions correct?
• Are integer overflow/underflow cases considered?
• Are switch statements exhaustive (including default case)?
• Are configuration macros used instead of hard-coded values?

2.3 RTOS Multi-Thread Safety