From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH v3] staging: fbtft: replace empty macro args with identity converter Date: Mon, 13 Apr 2026 18:50:48 +1000 Message-ID: In-Reply-To: <2026041338-guiding-concur-3831@gregkh> References: <2026041338-guiding-concur-3831@gregkh> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Verdict: The approach is correct but the commit message needs revision.** The change itself is sound: introducing `#define fbtft_no_conv(x) (x)` as a= n identity macro and using it in place of the empty argument: ```c +#define fbtft_no_conv(x) (x) ... -define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, ) +define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, fbtft_no_conv) ... -define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, u16, ) +define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, u16, fbtft_no_conv) ``` The expansion is functionally identical: `fbtft_no_conv((u8)va_arg(args, un= signed int))` expands to `((u8)va_arg(args, unsigned int))`, which is the s= ame result as the empty-argument expansion. **Issues:** 1. **Inaccurate commit message**: The commit message states *"Passing an em= pty token as modifier is undefined behavior in C for fixed-arity macros."* = This is incorrect. C99 (=C2=A76.10.3) explicitly permits empty macro argume= nts =E2=80=94 the argument count still matches (4 args to 4 params; the 4th= is just an empty token sequence). The existing code is technically valid C= 99. The change is still worthwhile for **clarity and readability** =E2=80= =94 making the intent explicit with a named identity conversion is better t= han relying on the subtle empty-argument expansion trick =E2=80=94 but the = justification should be corrected. Claiming UB when there is none weakens t= he argument for the patch. 2. **Minor style nit**: The `fbtft_no_conv` macro is placed above the `defi= ne_fbtft_write_reg` macro definition (after the file header comment block),= which is fine placement-wise. However, since `cpu_to_be16` is a kernel-pro= vided macro/function, it might be worth adding a brief comment like `/* ide= ntity conversion for cases that need no byte-order change */` to explain wh= y this exists, especially for a staging driver where cleanup patches are co= mmon and future readers may wonder what it's for. 3. **Process**: The `[PATCH v3]` label implies there were v1 and v2 before,= but no changelog below the `---` line explains what changed between versio= ns. For staging patches this is less critical, but it's good practice. --- Generated by Claude Code Patch Reviewer