A checkpatch modification was suggested privately about
braces around an if test like
if (foo)
;
so I thought I'd see how many of these possibly pointless
if tests exist. There aren't many.
Here are a couple in sibyte:
Maybe this should be documented as:
"well, don't know what to do here"
$ cat if_semi.cocci
@@
expression e;
@@
* if (e) ;
$ spatch --very-quiet -sp-file if_semi.cocci .
diff -u -p ./arch/mips/sibyte/common/cfe_console.c /tmp/nothing/arch/mips/sibyte/common/cfe_console.c
--- ./arch/mips/sibyte/common/cfe_console.c
+++ /tmp/nothing/arch/mips/sibyte/common/cfe_console.c
@@ -22,8 +22,6 @@ static void cfe_console_write(struct con
if (str[i] == '\n') {
do {
written = cfe_write(cfe_cons_handle, &str[last], i-last);
- if (written < 0)
- ;
last += written;
} while (last < i);
while (cfe_write(cfe_cons_handle, "\r", 1) <= 0)
@@ -33,8 +31,6 @@ static void cfe_console_write(struct con
if (last != count) {
do {
written = cfe_write(cfe_cons_handle, &str[last], count-last);
- if (written < 0)
- ;
last += written;
} while (last < count);
}
On 24/2/24 23:28, Joe Perches wrote:
> A checkpatch modification was suggested privately about
> braces around an if test like
>
> if (foo)
> ;
>
> so I thought I'd see how many of these possibly pointless
> if tests exist. There aren't many.
>
> Here are a couple in sibyte:
>
> Maybe this should be documented as:
>
> "well, don't know what to do here"
Or pr_devel() / pr_warn() if you want someone to notice it :)
>
> $ cat if_semi.cocci
> @@
> expression e;
> @@
>
> * if (e) ;
>
> $ spatch --very-quiet -sp-file if_semi.cocci .
> diff -u -p ./arch/mips/sibyte/common/cfe_console.c /tmp/nothing/arch/mips/sibyte/common/cfe_console.c
> --- ./arch/mips/sibyte/common/cfe_console.c
> +++ /tmp/nothing/arch/mips/sibyte/common/cfe_console.c
> @@ -22,8 +22,6 @@ static void cfe_console_write(struct con
> if (str[i] == '\n') {
> do {
> written = cfe_write(cfe_cons_handle, &str[last], i-last);
> - if (written < 0)
> - ;
> last += written;
> } while (last < i);
> while (cfe_write(cfe_cons_handle, "\r", 1) <= 0)
> @@ -33,8 +31,6 @@ static void cfe_console_write(struct con
> if (last != count) {
> do {
> written = cfe_write(cfe_cons_handle, &str[last], count-last);
> - if (written < 0)
> - ;
> last += written;
> } while (last < count);
> }
>
On Sat, 24 Feb 2024, Joe Perches wrote:
> Here are a couple in sibyte:
>
> Maybe this should be documented as:
>
> "well, don't know what to do here"
Of course just removing these useless conditionals isn't going to help
here because if a console write does fail for some reason, then the index
will move backwards regardless. At least we now have some kind of a
placeholder to bring someone's attention (such as yours).
> $ spatch --very-quiet -sp-file if_semi.cocci .
> diff -u -p ./arch/mips/sibyte/common/cfe_console.c /tmp/nothing/arch/mips/sibyte/common/cfe_console.c
> --- ./arch/mips/sibyte/common/cfe_console.c
> +++ /tmp/nothing/arch/mips/sibyte/common/cfe_console.c
> @@ -22,8 +22,6 @@ static void cfe_console_write(struct con
> if (str[i] == '\n') {
> do {
> written = cfe_write(cfe_cons_handle, &str[last], i-last);
> - if (written < 0)
> - ;
> last += written;
> } while (last < i);
> while (cfe_write(cfe_cons_handle, "\r", 1) <= 0)
The author indeed clearly was undecided as to what to do as the full last
statement here is actually:
while (cfe_write(cfe_cons_handle, "\r", 1) <= 0)
;
potentially looping indefinitely.
The CFE API clearly says:
Error codes:
Code Description
CFE_ERR_INV_PARAM File handle is invalid
others Device may return device-specific error codes
and maybe CFE never actually fails for a console device write (I'd have to
check the sources if there are any "others" for the console device and I'd
assume the console file handle is always valid), but IMO the safe approach
would be just to chicken out and silently return on a failure from any of
these calls. Also this code has been oddly written IMO and would benefit
from some refactoring. I'll see if I can queue a patch.
Maciej