General code cleanup #55

jonrebm · 2025-12-14T19:40:15Z

This pr contains a series of housekeeping patches.

Dropping a single unused int i is probably uncontroversial, removing lengthy license headers in favor of two SPDX lines and broad code reformatting maybe less so.

Changes where made to now pass most of -Wpedantic, where adaption seemed appropriate.

I used uncrustify to enforce some basic code style rules, and added the uncrustify config to version control. The chosen ruleset is very soft and tries to be close to kernel style. I only enabled uncrustify rules where I found all changes it lead to in the current codebase to be beneficial in every single instance.

To avoid the warning: ISO C does not permit named variadic macros [-Werror=variadic-macros] This is not a functional change. Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

They are nice but I don't think we're doing C23 yet and here, they aren't helping. To avoid the warning: use of an empty initializer is a C23 extension [-Werror,-Wc23-extensions] Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

To avoid the warning: arithmetic on a pointer to void is a GNU extension [-Werror,-Wgnu-pointer-arith] Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

GNU statement expressions are a nonstandard extension and the naive macros will suit us just fine as long as we use appropriate caution when using those macros, as we should with all macro functions. Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

This error handling prevents re-reading the same bytes if the buffer is exhausted. This also avoids the warning: ignoring return value of ‘read’ declared with attribute ‘warn_unused_result’ [-Werror=unused-result] Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

Avoids warning for -Werror=unused-variable Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

It's less prosy this way Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

Use consistent whitespace, identation etc. Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

ukleinek · 2025-12-16T14:31:32Z

commands_fsl_imx.c

-		u_char	*ucp = (u_char *)linebuf;
-		uint	count = 52;
+		char linebuf[DISP_LINE_LEN];
+		uint   *uip = (uint   *)linebuf;


I would replace all whitespace in this line with a single space (which matches the common style in the kernel).

ukleinek · 2025-12-16T14:32:36Z

microcom.c

 #include "config.h"

-static struct termios sots;	/* old stdout/in termios settings to restore */
+static struct termios sots;     /* old stdout/in termios settings to restore */


again a single space? (Or move the comment above the code line?)

ukleinek · 2025-12-16T14:35:59Z

can.c

- *
- */
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: 2010 by Marc Kleine-Budde <mkl@pengutronix.de>


This is the only line that has a "by", maybe unify that and drop it.

ukleinek · 2025-12-16T14:42:12Z

commands_fsl_imx.c

-	for (i = 0; i < 15; i++) {
-		read(ios->fd, &buf[i], 1);
+	if (read(ios->fd, buf, 15) < 15) {
+		printf("read error\n");


The more correct handling would be to read 7 bytes if the first read only gave you 8, right?

I guess you didn't test that and an alternative to fixing warnings in this code is to just drop the whole file?

ukleinek · 2025-12-16T14:47:04Z

microcom.h

-        (void) (&_max1 == &_max2);              \
-        _max1 > _max2 ? _max1 : _max2; })
+#define min(a, b) ((a) < (b) ? (a) : (b))
+#define max(a, b) ((a) > (b) ? (a) : (b))


max is unused, so drop it?

I wonder what the goal is here? Do you want to compile microcom on a platform where you prefer using a compiler that doesn't support statement expressions? My gut feeling would be to keep the safeguards unless there is a better reason than preemptively prepare for a new platform/compiler. I don't trust developers to understand the corner cases of a macro that looks like a function.

ukleinek · 2025-12-16T14:48:29Z

microcom.c

 				break;
 			case 'a':
-				answerback = optarg;
+				answerback = (unsigned char *) optarg;


Does the line (before the change) also result in a warning? answerback's type is plain char, so I wonder why the unsigned is added here.

ukleinek · 2025-12-16T14:54:32Z

microcom.c


 	if (answerback) {
-		ret = asprintf(&answerback, "%s\n", answerback);
+		ret = asprintf((char **) &answerback, "%s\n", answerback);


Huh, really? answerback is a char, so &answerback is already a char **?

Having said that I think that asprintf is strange and I'd do

diff --git a/microcom.c b/microcom.c index a8810b4fd3c8..13b87146c034 100644 --- a/microcom.c +++ b/microcom.c @@ -198,12 +198,6 @@ int main(int argc, char *argv[]) if (optind < argc) main_usage(1, "", ""); - if (answerback) { - ret = asprintf(&answerback, "%s\n", answerback); - if (ret < 0) - exit (1); - } - commands_init(); commands_fsl_imx_init(); diff --git a/mux.c b/mux.c index d15012bf26cc..7bde864020f4 100644 --- a/mux.c +++ b/mux.c @@ -45,9 +45,10 @@ static int handle_receive_buf(struct ios_ops *ios, unsigned char *buf, int len) switch (*buf) { case 5: write_receive_buf(sendbuf, buf - sendbuf); - if (answerback) + if (answerback) { ios->write(ios, answerback, strlen(answerback)); - else + ios->write(ios, "\n", 1); + } else write_receive_buf(buf, 1); buf += 1;

instead.

ukleinek · 2025-12-16T14:55:54Z

microcom.h

 extern int opt_force;
 extern int listenonly;
-extern char *answerback;
+extern unsigned char *answerback;


Ah, you change the type of answerback, this makes some of the feedback above wrong, but this doesn't fit into the commit's topic, does it?

ukleinek · 2025-12-16T14:58:38Z

microcom.c

 		{ "answerback", required_argument, NULL, 'a'},
 		{ "version", no_argument, NULL, 'v' },
-		{ },
+		{ 0, 0, 0, 0 },


Uh, I'd say either use { 0 } here if you don't like the c23 stuff, or use NULL instead of 0 to initialize pointers.

ukleinek · 2025-12-16T15:01:01Z

microcom.h

 int do_script(char *script);

-#define dbg_printf(fmt,args...)  ({ if (debug) printf(fmt ,##args); })
+#define dbg_printf(...) do { if (debug) printf(__VA_ARGS__); } while(0)


The commit log doesn't mention that you also drop a GNU statement expression.

Oops well the commit log does mention it, it just ended up in the wrong commit 3e6ec3e which I'll fix.

I'm not sure if all of these changes are for the better. To me, GNU extension sounds like something which is better avoided but then again, so should do {...} while(0).
What do you think?

I like statement expressions better than do { ... } while(0). If you don't like both, there is still the option to make dbg_printf a static inline function calling vprintf()

jonrebm added 9 commits December 14, 2025 00:44

Do not use named variadic macros

c6a86f8

To avoid the warning: ISO C does not permit named variadic macros [-Werror=variadic-macros] This is not a functional change. Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

Do not use c23 empty initializers

6112a0b

They are nice but I don't think we're doing C23 yet and here, they aren't helping. To avoid the warning: use of an empty initializer is a C23 extension [-Werror,-Wc23-extensions] Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

Don't do arithmetic on void pointers

e998b60

To avoid the warning: arithmetic on a pointer to void is a GNU extension [-Werror,-Wgnu-pointer-arith] Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

commands_fsl_imx.c: drop unused variable i

53504fe

Avoids warning for -Werror=unused-variable Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

treewide: Use SPDX license headers

452d793

It's less prosy this way Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

treewide: Format using uncrustify and add uncrustify.cfg

698d29d

Use consistent whitespace, identation etc. Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

microcom.c: Do not use uninitialized memory

331c38c

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

jonrebm mentioned this pull request Dec 14, 2025

Pytest tests and CI #58

Draft

ukleinek suggested changes Dec 16, 2025

View reviewed changes

General code cleanup #55

Are you sure you want to change the base?

General code cleanup #55

Uh oh!

Conversation

jonrebm commented Dec 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants