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] staging: fbtft: avoid empty macro argument Date: Tue, 31 Mar 2026 17:56:51 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Missing commit message body.** The patch has no description at all =E2=80= =94 just a Signed-off-by. It should explain *why* this change is being made= (e.g., "Empty macro arguments are undefined behavior per ISO C99 =C2=A76.1= 0.3 and trigger warnings with `-Wmissing-macro-arg`" or similar). A reviewe= r or future reader of `git log` needs to understand the motivation. **The fix itself is correct.** The `modifier` parameter appears in three pl= aces in the macro body: ```c buf[i] =3D modifier((data_type)va_arg(args, unsigned int)); *buf =3D modifier((data_type)va_arg(args, unsigned int)); *buf++ =3D modifier((data_type)va_arg(args, unsigned int)); ``` Using `fbtft_nop(x)` defined as `(x)` correctly preserves the original sema= ntics =E2=80=94 it's an identity transformation, matching what the empty ar= gument was implicitly doing. **Naming/style nit:** The name `fbtft_nop` is slightly misleading =E2=80=94= "nop" typically implies "no operation" (i.e., a statement with no effect),= whereas this is an identity *function*. Something like `fbtft_identity` wo= uld be more descriptive, though this is minor and a matter of taste. **Minor: `#define` placement is fine.** Placing the `#define` between the m= acro definition and its first use is reasonable. Since this is a file-scope= d macro only used in this translation unit, it doesn't need to go in a head= er. **Recommendation:** The patch needs a proper commit message. The code chang= e itself is a reasonable cleanup. With a commit message added, this would b= e acceptable. --- Generated by Claude Code Patch Reviewer