diff mbox series

lib/vsprintf: Remove redundant code

Message ID 20231018064817.86721-1-jiapeng.chong@linux.alibaba.com
State New
Headers
Series lib/vsprintf: Remove redundant code |

Commit Message

Jiapeng Chong Oct. 18, 2023, 6:48 a.m. UTC 
  When variable needcolon is assigned a value of false, it must be
assigned a value of true later on, which is redundant code.

lib/vsprintf.c:1411:4: warning: Value stored to 'needcolon' is never read.

Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=6909
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
 lib/vsprintf.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Oct. 18, 2023, 12:56 p.m. UTC | #1 
On Wed, Oct 18, 2023 at 02:48:17PM +0800, Jiapeng Chong wrote:
> When variable needcolon is assigned a value of false, it must be
> assigned a value of true later on, which is redundant code.
> 
> lib/vsprintf.c:1411:4: warning: Value stored to 'needcolon' is never read.

...

> @@ -1406,10 +1406,9 @@ char *ip6_compressed_string(char *p, const char *addr)
>  			i += longest - 1;
>  			continue;
>  		}
> -		if (needcolon) {
> +		if (needcolon)
>  			*p++ = ':';
> -			needcolon = false;
> -		}
> +
>  		/* hex u16 without leading 0s */
>  		word = ntohs(in6.s6_addr16[i]);
>  		hi = word >> 8;

Logically you may remove then the assignment to true as well.

But I would double check if it's not a continue missing somewhere or so.
Rasmus Villemoes Oct. 19, 2023, 11:51 a.m. UTC | #2 
On 18/10/2023 08.48, Jiapeng Chong wrote:
> When variable needcolon is assigned a value of false, it must be
> assigned a value of true later on, which is redundant code.

NAK on any patch modifying these things without first filling the blank
space in ip6() in lib/test_printf.c with tests for all the various ip6
printing modes.

Adding test cases there is much more useful than code-golfing, and would
help verify that these changes are actually ok. Or may reveal that the
current code is buggy.

Rasmus
Andy Shevchenko Oct. 19, 2023, 12:21 p.m. UTC | #3 
On Thu, Oct 19, 2023 at 01:51:45PM +0200, Rasmus Villemoes wrote:
> On 18/10/2023 08.48, Jiapeng Chong wrote:

...

>  Or may reveal that the current code is buggy.

...and hopefully didn't become a part of any ABIs...
Petr Mladek Oct. 19, 2023, 1:21 p.m. UTC | #4 
On Wed 2023-10-18 14:48:17, Jiapeng Chong wrote:
> When variable needcolon is assigned a value of false, it must be
> assigned a value of true later on, which is redundant code.
> 
> lib/vsprintf.c:1411:4: warning: Value stored to 'needcolon' is never read.
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=6909
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
>  lib/vsprintf.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index afb88b24fa74..9a9086885da8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1406,10 +1406,9 @@ char *ip6_compressed_string(char *p, const char *addr)
>  			i += longest - 1;
>  			continue;
>  		}
> -		if (needcolon) {
> +		if (needcolon)
>  			*p++ = ':';
> -			needcolon = false;
> -		}
> +
>  		/* hex u16 without leading 0s */
>  		word = ntohs(in6.s6_addr16[i]);
>  		hi = word >> 8;

I am against this change. The assignment should be there from the
logical POV. IMHO, it helps people to verify that the code works
as expected. The removal might even cause regression when the logic
gets modified in the future.

Anyway, this is a slow path. The code readability is more important
then the speed. Let compiler do these micro optimizations.

Best Regards,
Petr
diff mbox series

Patch

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index afb88b24fa74..9a9086885da8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1406,10 +1406,9 @@  char *ip6_compressed_string(char *p, const char *addr)
 			i += longest - 1;
 			continue;
 		}
-		if (needcolon) {
+		if (needcolon)
 			*p++ = ':';
-			needcolon = false;
-		}
+
 		/* hex u16 without leading 0s */
 		word = ntohs(in6.s6_addr16[i]);
 		hi = word >> 8;