vsprintf: Drop unused assignment of fmt.state

[PATCH] vsprintf: Drop unused assignment of fmt.state

Posted by I Hsin Cheng 1 year ago

Remove unused assignment of "fmt.state", in both cases the value of
"fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
"FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
after the assignment.

Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
---
 lib/vsprintf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 56fe96319292..400299465875 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2569,7 +2569,7 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 			spec->field_width = -spec->field_width;
 			spec->flags |= LEFT;
 		}
-		fmt.state = FORMAT_STATE_NONE;
+
 		goto precision;
 	}
 
@@ -2578,7 +2578,7 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 		if (spec->precision < 0)
 			spec->precision = 0;
 
-		fmt.state = FORMAT_STATE_NONE;
+
 		goto qualifier;
 	}
 
-- 
2.43.0

Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

Posted by Andy Shevchenko 1 year ago

On Thu, Feb 06, 2025 at 01:25:07AM +0800, I Hsin Cheng wrote:
> Remove unused assignment of "fmt.state", in both cases the value of
> "fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
> "FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
> after the assignment.

...

> struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)

>  			spec->field_width = -spec->field_width;
>  			spec->flags |= LEFT;
>  		}
> -		fmt.state = FORMAT_STATE_NONE;
> +
>  		goto precision;
>  	}
>  

While both are kinda redundant, this is not obvious what's stated in the commit
message. Yes, `goto qualifier;` is straightforward, but not `goto precision;`.
Which makes me think that these assignments can make code robust against
potential future changes to allow to catch up the wrong code paths.

Whatever maintainers decide, technically the change looks correct to me.

...

>  		if (spec->precision < 0)
>  			spec->precision = 0;
>  
> -		fmt.state = FORMAT_STATE_NONE;
> +

What's the reason to add an extra blank line? (We have already one here)

>  		goto qualifier;
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

Posted by Petr Mladek 12 months ago

On Thu 2025-02-06 17:32:32, Andy Shevchenko wrote:
> On Thu, Feb 06, 2025 at 01:25:07AM +0800, I Hsin Cheng wrote:
> > Remove unused assignment of "fmt.state", in both cases the value of
> > "fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
> > "FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
> > after the assignment.
> 
> ...
> 
> > struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> 
> >  			spec->field_width = -spec->field_width;
> >  			spec->flags |= LEFT;
> >  		}
> > -		fmt.state = FORMAT_STATE_NONE;
> > +
> >  		goto precision;
> >  	}
> >  
> 
> While both are kinda redundant, this is not obvious what's stated in the commit
> message. Yes, `goto qualifier;` is straightforward, but not `goto precision;`.
> Which makes me think that these assignments can make code robust against
> potential future changes to allow to catch up the wrong code paths.

I fully agree with Andy here.

That said, I see the following right below the two conditions modified
in this patch:

	/* By default */
	fmt.state = FORMAT_STATE_NONE;

A good solution would be to move it up. It will be then obvious
that we could remove these two initializations. I mean
to do the following:

--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2563,13 +2563,15 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 	const char *start = fmt.str;
 	char flag;
 
+	/* By default */
+	fmt.state = FORMAT_STATE_NONE;
+
 	/* we finished early by reading the field width */
 	if (unlikely(fmt.state == FORMAT_STATE_WIDTH)) {
 		if (spec->field_width < 0) {
 			spec->field_width = -spec->field_width;
 			spec->flags |= LEFT;
 		}
-		fmt.state = FORMAT_STATE_NONE;
 		goto precision;
 	}
 
@@ -2578,13 +2580,9 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 		if (spec->precision < 0)
 			spec->precision = 0;
 
-		fmt.state = FORMAT_STATE_NONE;
 		goto qualifier;
 	}
 
-	/* By default */
-	fmt.state = FORMAT_STATE_NONE;
-
 	for (; *fmt.str ; fmt.str++) {
 		if (*fmt.str == '%')
 			break;


Best Regards,
Petr

Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

Posted by Andy Shevchenko 12 months ago

On Mon, Feb 10, 2025 at 02:09:53PM +0100, Petr Mladek wrote:
> On Thu 2025-02-06 17:32:32, Andy Shevchenko wrote:
> > On Thu, Feb 06, 2025 at 01:25:07AM +0800, I Hsin Cheng wrote:
> > > Remove unused assignment of "fmt.state", in both cases the value of
> > > "fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
> > > "FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
> > > after the assignment.

...

> > > struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> > 
> > >  			spec->field_width = -spec->field_width;
> > >  			spec->flags |= LEFT;
> > >  		}
> > > -		fmt.state = FORMAT_STATE_NONE;
> > > +
> > >  		goto precision;
> > >  	}
> > >  
> > 
> > While both are kinda redundant, this is not obvious what's stated in the commit
> > message. Yes, `goto qualifier;` is straightforward, but not `goto precision;`.
> > Which makes me think that these assignments can make code robust against
> > potential future changes to allow to catch up the wrong code paths.
> 
> I fully agree with Andy here.
> 
> That said, I see the following right below the two conditions modified
> in this patch:
> 
> 	/* By default */
> 	fmt.state = FORMAT_STATE_NONE;
> 
> A good solution would be to move it up. It will be then obvious
> that we could remove these two initializations. I mean
> to do the following:

Which can't be performed (one need to check the old value first somehow) :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

Posted by Petr Mladek 12 months ago

On Mon 2025-02-10 17:08:25, Andy Shevchenko wrote:
> On Mon, Feb 10, 2025 at 02:09:53PM +0100, Petr Mladek wrote:
> > On Thu 2025-02-06 17:32:32, Andy Shevchenko wrote:
> > > On Thu, Feb 06, 2025 at 01:25:07AM +0800, I Hsin Cheng wrote:
> > > > Remove unused assignment of "fmt.state", in both cases the value of
> > > > "fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
> > > > "FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
> > > > after the assignment.
> 
> ...
> 
> > > > struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> > > 
> > > >  			spec->field_width = -spec->field_width;
> > > >  			spec->flags |= LEFT;
> > > >  		}
> > > > -		fmt.state = FORMAT_STATE_NONE;
> > > > +
> > > >  		goto precision;
> > > >  	}
> > > >  
> > > 
> > > While both are kinda redundant, this is not obvious what's stated in the commit
> > > message. Yes, `goto qualifier;` is straightforward, but not `goto precision;`.
> > > Which makes me think that these assignments can make code robust against
> > > potential future changes to allow to catch up the wrong code paths.
> > 
> > I fully agree with Andy here.
> > 
> > That said, I see the following right below the two conditions modified
> > in this patch:
> > 
> > 	/* By default */
> > 	fmt.state = FORMAT_STATE_NONE;
> > 
> > A good solution would be to move it up. It will be then obvious
> > that we could remove these two initializations. I mean
> > to do the following:
> 
> Which can't be performed (one need to check the old value first somehow) :-)

Grr, humph /o\

OK, I would personally keep the code as it is now. I do not see any
big benefit in removing the duplicity assignment.

IMHO, the assignment make the code more robust for future changes.
Let the compiler to optimize it out.

Best Regards,
Petr

Re: [PATCH] vsprintf: Drop unused assignment of fmt.state

Posted by I Hsin Cheng 12 months ago

On Thu, Feb 06, 2025 at 05:32:32PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 06, 2025 at 01:25:07AM +0800, I Hsin Cheng wrote:
> > Remove unused assignment of "fmt.state", in both cases the value of
> > "fmt.state" will be overwritten by either "FORMAT_STATE_PRECISION" or
> > "FORMAT_STATE_NUM", the value "FORMAT_STATE_NONE" isn't going to be used
> > after the assignment.
> 
> ...
> 
> > struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> 
> >  			spec->field_width = -spec->field_width;
> >  			spec->flags |= LEFT;
> >  		}
> > -		fmt.state = FORMAT_STATE_NONE;
> > +
> >  		goto precision;
> >  	}
> >  
> 
> While both are kinda redundant, this is not obvious what's stated in the commit
> message. Yes, `goto qualifier;` is straightforward, but not `goto precision;`.
> Which makes me think that these assignments can make code robust against
> potential future changes to allow to catch up the wrong code paths.
> 
> Whatever maintainers decide, technically the change looks correct to me.
> 
> ...
> 
> >  		if (spec->precision < 0)
> >  			spec->precision = 0;
> >  
> > -		fmt.state = FORMAT_STATE_NONE;
> > +
> 
> What's the reason to add an extra blank line? (We have already one here)
> 
> >  		goto qualifier;
> >  	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Hi Andy,

Thanks for your kindly review!

> What's the reason to add an extra blank line? (We have already one here)

Well I think I didn't intentionally add a blank line here, I simply
delete the assigment but left the line there. I'll have it deleted as
well if the maintainers decided to make any change.

Best regards,
I Hsin Cheng