From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/nouveau: fix misleading indentation Date: Sat, 14 Mar 2026 07:24:32 +1000 Message-ID: In-Reply-To: <6c1d96782664e4f491f23e8c73545fe4d8b9533a.1773383224.git.joeypabalinas@gmail.com> References: <6c1d96782664e4f491f23e8c73545fe4d8b9533a.1773383224.git.joeypabalinas@gmail.com> <6c1d96782664e4f491f23e8c73545fe4d8b9533a.1773383224.git.joeypabalinas@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 **Hunk 1 (line 425): Semantically neutral but stylistically worse** ```c if (device->info.family >=3D NV_DEVICE_INFO_V0_TESLA) init->pushbuf_domains =3D NOUVEAU_GEM_DOMAIN_VRAM | NOUVEAU_GEM_DOMAIN_GART; else - if (chan->chan->push.buffer->bo.resource->mem_type =3D=3D TTM_PL_VRAM) - init->pushbuf_domains =3D NOUVEAU_GEM_DOMAIN_VRAM; - else - init->pushbuf_domains =3D NOUVEAU_GEM_DOMAIN_GART; + if (chan->chan->push.buffer->bo.resource->mem_type =3D=3D TTM_PL_VRAM) + init->pushbuf_domains =3D NOUVEAU_GEM_DOMAIN_VRAM; + else + init->pushbuf_domains =3D NOUVEAU_GEM_DOMAIN_GART; ``` The original code is an `if / else if / else` chain =E2=80=94 three mutuall= y exclusive branches at the same logical level. The patch nests the second = and third branches inside the first `else`, making it look like a two-level= decision tree. While the **runtime behavior** is identical (C doesn't care= about indentation), the new indentation is arguably **more** misleading be= cause it obscures the flat if/else-if/else structure. The original style (`= else` on its own line followed by `if` at the original indent level) is a d= eliberate pattern used throughout the nouveau driver and the kernel at larg= e. **Hunk 2 (line 579): FUNCTIONAL REGRESSION =E2=80=94 changes code behavior** ```c } else - if ((init->class & 0x00ff) =3D=3D 0x00b3) { /* msppp */ - ... - } else { - oclass =3D init->class; - } + if ((init->class & 0x00ff) =3D=3D 0x00b3) { /* msppp */ + ... + } else + oclass =3D init->class; ``` This is the most critical problem. Looking at the full context of the funct= ion, the original code has a **flat** `if / else if / else if / else if / e= lse` chain checking `init->class` against `0x006e`, `0x00b1`, `0x00b2`, `0x= 00b3`, with a final `else { oclass =3D init->class; }` as the default fallb= ack. The patch only modifies the **last** `else if` in this chain (the `0x00b3` = case) and the final `else` default. By indenting these, the `else oclass = =3D init->class` now becomes the else-branch of only the `0x00b3` test, rat= her than the default for the entire chain. Looking at the unmodified preced= ing code (lines 594-611 in the patched file), the `0x00b1` and `0x00b2` cas= es still use the original `} else\nif (...)` pattern at the original indent= level. This creates an **inconsistent and broken** if/else chain: - The `0x006e`, `0x00b1`, `0x00b2` blocks remain at the original indent lev= el - The `0x00b3` block is now nested inside the `0x00b2` else - The default `oclass =3D init->class` is now the else of only the `0x00b3`= test, not the whole chain Since C treats `else if` as `else { if ... }`, the **semantics haven't actu= ally changed** in terms of which branch executes. However, the patch create= s a deeply inconsistent style: the first three `else if` branches use the o= riginal pattern while the last one is indented differently. If a future dev= eloper reads this code, the mixed indentation would be genuinely confusing.= And the removal of the braces around `{ oclass =3D init->class; }` is a gr= atuitous style change. **Hunk 3 (line 652): Semantically neutral but stylistically worse** ```c } else - if (drm->agp.bridge) { - ... - } else { - ... - } + if (drm->agp.bridge) { + ... + } else { + ... + } ``` Same issue as Hunk 1 =E2=80=94 the original is a flat `if / else if / else`= chain and the patch makes it look nested. No behavioral change but the new= indentation misrepresents the logical structure. **Summary of issues:** 1. The original indentation style (`else\nif`) is a **deliberate and w= idely-used kernel coding pattern** for flat if/else-if chains when the firs= t `if` has a single-statement body (no braces). The patch author has miside= ntified this as a bug. 2. Hunk 2 creates **inconsistent indentation** within a single if/else-if c= hain =E2=80=94 only the last branch is re-indented while the preceding bran= ches retain the original style. 3. The deeper nesting makes the code **harder to read**, not easier, by obs= curing the flat control flow. 4. The commit message ("Fix misleading indentation in if/else statements") = provides no analysis of why the indentation is misleading or what bug it mi= ght cause. **Recommendation:** NAK this patch. The existing indentation is intentional= and correct for this codebase. --- Generated by Claude Code Patch Reviewer