From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dyndbg: factor ddebug_match_desc out from ddebug_change Date: Sat, 14 Mar 2026 07:02:59 +1000 Message-ID: In-Reply-To: <20260313131045.2526422-3-jim.cromie@gmail.com> References: <20260313131045.2526422-1-jim.cromie@gmail.com> <20260313131045.2526422-3-jim.cromie@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Commit message quality:** Good. Clearly explains the motivation =E2=80=94= extracting the inner-loop filter logic into a separate function for readab= ility. **Review of the changes:** This is a straightforward, clean code refactor. The new function: ```c static bool ddebug_match_desc(const struct ddebug_query *query, struct _ddebug *dp, int valid_class) ``` extracts exactly the per-descriptor matching logic from `ddebug_change()`, = converting each `continue` to `return false`. The transformation is correct= =E2=80=94 each filter condition is preserved identically. **Minor observations:** 1. The `dp` parameter could be `const`: ```c struct _ddebug *dp, ``` Since `ddebug_match_desc` only reads from `dp`, marking it `const struct _d= debug *dp` would better express intent. This is a nit, not a blocker. 2. The commit message says "also fix checkpatch complaints about spaces and= braces." Looking at the diff, the only style change I can see is the spaci= ng around `query->format+1` =E2=86=92 `query->format + 1` (line 702 vs the = removed line 750). The commit message should either enumerate what style fi= xes were made or just omit this note if it's a single trivial whitespace ch= ange. Mixing style fixes with refactoring in one commit is generally accept= able when the style fix is trivial like this one. 3. The function placement is good =E2=80=94 it's placed immediately above `= ddebug_change()` where it's called, with the existing block comment acting = as a natural separator. **Verdict:** Clean, correct refactoring. No functional changes. Reviewed-by= worthy. --- Generated by Claude Code Patch Reviewer