[U-Boot] cmd: avoid decimal conversion
diff mbox series

Message ID 068609d5f2936454d3a999de99317f1def78ceea.1568209191.git.michal.simek@xilinx.com
State New
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot] cmd: avoid decimal conversion
Related show

Commit Message

Michal Simek Sept. 11, 2019, 1:39 p.m. UTC
From: T Karthik Reddy <t.karthik.reddy@xilinx.com>

This patch uses auto instead of decimal in simple_strtoul().

Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 cmd/test.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

Tom Rini Sept. 13, 2019, 3:09 p.m. UTC | #1
On Wed, Sep 11, 2019 at 03:39:53PM +0200, Michal Simek wrote:

> From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> 
> This patch uses auto instead of decimal in simple_strtoul().
> 
> Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>  cmd/test.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/cmd/test.c b/cmd/test.c
> index fa0c349f0827..258bfd880653 100644
> --- a/cmd/test.c
> +++ b/cmd/test.c
> @@ -113,28 +113,28 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  			expr = strcmp(ap[0], ap[2]) > 0;
>  			break;
>  		case OP_INT_EQ:
> -			expr = simple_strtol(ap[0], NULL, 10) ==
> -					simple_strtol(ap[2], NULL, 10);
> +			expr = simple_strtol(ap[0], NULL, 0) ==
> +					simple_strtol(ap[2], NULL, 0);
>  			break;
>  		case OP_INT_NEQ:
> -			expr = simple_strtol(ap[0], NULL, 10) !=
> -					simple_strtol(ap[2], NULL, 10);
> +			expr = simple_strtol(ap[0], NULL, 0) !=
> +					simple_strtol(ap[2], NULL, 0);
>  			break;
>  		case OP_INT_LT:
> -			expr = simple_strtol(ap[0], NULL, 10) <
> -					simple_strtol(ap[2], NULL, 10);
> +			expr = simple_strtol(ap[0], NULL, 0) <
> +					simple_strtol(ap[2], NULL, 0);
>  			break;
>  		case OP_INT_LE:
> -			expr = simple_strtol(ap[0], NULL, 10) <=
> -					simple_strtol(ap[2], NULL, 10);
> +			expr = simple_strtol(ap[0], NULL, 0) <=
> +					simple_strtol(ap[2], NULL, 0);
>  			break;
>  		case OP_INT_GT:
> -			expr = simple_strtol(ap[0], NULL, 10) >
> -					simple_strtol(ap[2], NULL, 10);
> +			expr = simple_strtol(ap[0], NULL, 0) >
> +					simple_strtol(ap[2], NULL, 0);
>  			break;
>  		case OP_INT_GE:
> -			expr = simple_strtol(ap[0], NULL, 10) >=
> -					simple_strtol(ap[2], NULL, 10);
> +			expr = simple_strtol(ap[0], NULL, 0) >=
> +					simple_strtol(ap[2], NULL, 0);
>  			break;
>  		case OP_FILE_EXISTS:
>  			expr = file_exists(ap[1], ap[2], ap[3], FS_TYPE_ANY);

I'm going to NAK this, but could be argued around to changing my mind.
While it's true that in general command inputs are hex and not decimal,
this has been decimal since introduction in 2009.  So changing it now is
breaking ABI and other peoples test scripts, so I don't think we can do
this, sorry.
Michael Trimarchi Sept. 13, 2019, 3:17 p.m. UTC | #2
On Fri, Sep 13, 2019 at 5:10 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Sep 11, 2019 at 03:39:53PM +0200, Michal Simek wrote:
>
> > From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> >
> > This patch uses auto instead of decimal in simple_strtoul().
> >
> > Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >
> >  cmd/test.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/cmd/test.c b/cmd/test.c
> > index fa0c349f0827..258bfd880653 100644
> > --- a/cmd/test.c
> > +++ b/cmd/test.c
> > @@ -113,28 +113,28 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >                       expr = strcmp(ap[0], ap[2]) > 0;
> >                       break;
> >               case OP_INT_EQ:
> > -                     expr = simple_strtol(ap[0], NULL, 10) ==
> > -                                     simple_strtol(ap[2], NULL, 10);
> > +                     expr = simple_strtol(ap[0], NULL, 0) ==
> > +                                     simple_strtol(ap[2], NULL, 0);
> >                       break;
> >               case OP_INT_NEQ:
> > -                     expr = simple_strtol(ap[0], NULL, 10) !=
> > -                                     simple_strtol(ap[2], NULL, 10);
> > +                     expr = simple_strtol(ap[0], NULL, 0) !=
> > +                                     simple_strtol(ap[2], NULL, 0);
> >                       break;
> >               case OP_INT_LT:
> > -                     expr = simple_strtol(ap[0], NULL, 10) <
> > -                                     simple_strtol(ap[2], NULL, 10);
> > +                     expr = simple_strtol(ap[0], NULL, 0) <
> > +                                     simple_strtol(ap[2], NULL, 0);
> >                       break;
> >               case OP_INT_LE:
> > -                     expr = simple_strtol(ap[0], NULL, 10) <=
> > -                                     simple_strtol(ap[2], NULL, 10);
> > +                     expr = simple_strtol(ap[0], NULL, 0) <=
> > +                                     simple_strtol(ap[2], NULL, 0);
> >                       break;
> >               case OP_INT_GT:
> > -                     expr = simple_strtol(ap[0], NULL, 10) >
> > -                                     simple_strtol(ap[2], NULL, 10);
> > +                     expr = simple_strtol(ap[0], NULL, 0) >
> > +                                     simple_strtol(ap[2], NULL, 0);
> >                       break;
> >               case OP_INT_GE:
> > -                     expr = simple_strtol(ap[0], NULL, 10) >=
> > -                                     simple_strtol(ap[2], NULL, 10);
> > +                     expr = simple_strtol(ap[0], NULL, 0) >=
> > +                                     simple_strtol(ap[2], NULL, 0);
> >                       break;
> >               case OP_FILE_EXISTS:
> >                       expr = file_exists(ap[1], ap[2], ap[3], FS_TYPE_ANY);
>
> I'm going to NAK this, but could be argued around to changing my mind.
> While it's true that in general command inputs are hex and not decimal,
> this has been decimal since introduction in 2009.  So changing it now is
> breaking ABI and other peoples test scripts, so I don't think we can do
> this, sorry.

Well, I think that this is automatic. There should be a back compatibility
with the ABI if base is 10

Michael

>
> --
> Tom
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot



--
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |
Michal Simek Sept. 19, 2019, 1:28 p.m. UTC | #3
On 13. 09. 19 17:09, Tom Rini wrote:
> On Wed, Sep 11, 2019 at 03:39:53PM +0200, Michal Simek wrote:
> 
>> From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
>>
>> This patch uses auto instead of decimal in simple_strtoul().
>>
>> Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>  cmd/test.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/cmd/test.c b/cmd/test.c
>> index fa0c349f0827..258bfd880653 100644
>> --- a/cmd/test.c
>> +++ b/cmd/test.c
>> @@ -113,28 +113,28 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  			expr = strcmp(ap[0], ap[2]) > 0;
>>  			break;
>>  		case OP_INT_EQ:
>> -			expr = simple_strtol(ap[0], NULL, 10) ==
>> -					simple_strtol(ap[2], NULL, 10);
>> +			expr = simple_strtol(ap[0], NULL, 0) ==
>> +					simple_strtol(ap[2], NULL, 0);
>>  			break;
>>  		case OP_INT_NEQ:
>> -			expr = simple_strtol(ap[0], NULL, 10) !=
>> -					simple_strtol(ap[2], NULL, 10);
>> +			expr = simple_strtol(ap[0], NULL, 0) !=
>> +					simple_strtol(ap[2], NULL, 0);
>>  			break;
>>  		case OP_INT_LT:
>> -			expr = simple_strtol(ap[0], NULL, 10) <
>> -					simple_strtol(ap[2], NULL, 10);
>> +			expr = simple_strtol(ap[0], NULL, 0) <
>> +					simple_strtol(ap[2], NULL, 0);
>>  			break;
>>  		case OP_INT_LE:
>> -			expr = simple_strtol(ap[0], NULL, 10) <=
>> -					simple_strtol(ap[2], NULL, 10);
>> +			expr = simple_strtol(ap[0], NULL, 0) <=
>> +					simple_strtol(ap[2], NULL, 0);
>>  			break;
>>  		case OP_INT_GT:
>> -			expr = simple_strtol(ap[0], NULL, 10) >
>> -					simple_strtol(ap[2], NULL, 10);
>> +			expr = simple_strtol(ap[0], NULL, 0) >
>> +					simple_strtol(ap[2], NULL, 0);
>>  			break;
>>  		case OP_INT_GE:
>> -			expr = simple_strtol(ap[0], NULL, 10) >=
>> -					simple_strtol(ap[2], NULL, 10);
>> +			expr = simple_strtol(ap[0], NULL, 0) >=
>> +					simple_strtol(ap[2], NULL, 0);
>>  			break;
>>  		case OP_FILE_EXISTS:
>>  			expr = file_exists(ap[1], ap[2], ap[3], FS_TYPE_ANY);
> 
> I'm going to NAK this, but could be argued around to changing my mind.
> While it's true that in general command inputs are hex and not decimal,
> this has been decimal since introduction in 2009.  So changing it now is
> breaking ABI and other peoples test scripts, so I don't think we can do
> this, sorry.

I also think that this is not breaking any ABI. test_hush_if_test.py is
around for a while to capture issues in this space and I can't see any
single failure in connection to this change.

If this accepted then we can add more tests like this
    ('test 0x2000000 -gt 0x2000001', False),
    ('test 0x2000000 -gt 0x2000000', False),
    ('test 0x2000000 -gt 0x1ffffff', True),
    ('test 2000000 -gt 0x1ffffff', False),
    ('test 0x2000000 -gt 1ffffff', True),

    ('test 0x2000000 -lt 1ffffff', False),
    ('test 0x2000000 -eq 2000000', False),
    ('test 0x2000000 -ne 2000000', True),

where some of them are failing without this patch
... test_hush_if_test[test 0x2000000 -gt 0x1ffffff-True]

... test_hush_if_test[test 2000000 -gt 0x1ffffff-False]

... test_hush_if_test[test 0x2000000 -gt 1ffffff-True]

... test_hush_if_test[test 0x2000000 -lt 1ffffff-False]


Thanks,
Michal

Patch
diff mbox series

diff --git a/cmd/test.c b/cmd/test.c
index fa0c349f0827..258bfd880653 100644
--- a/cmd/test.c
+++ b/cmd/test.c
@@ -113,28 +113,28 @@  static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			expr = strcmp(ap[0], ap[2]) > 0;
 			break;
 		case OP_INT_EQ:
-			expr = simple_strtol(ap[0], NULL, 10) ==
-					simple_strtol(ap[2], NULL, 10);
+			expr = simple_strtol(ap[0], NULL, 0) ==
+					simple_strtol(ap[2], NULL, 0);
 			break;
 		case OP_INT_NEQ:
-			expr = simple_strtol(ap[0], NULL, 10) !=
-					simple_strtol(ap[2], NULL, 10);
+			expr = simple_strtol(ap[0], NULL, 0) !=
+					simple_strtol(ap[2], NULL, 0);
 			break;
 		case OP_INT_LT:
-			expr = simple_strtol(ap[0], NULL, 10) <
-					simple_strtol(ap[2], NULL, 10);
+			expr = simple_strtol(ap[0], NULL, 0) <
+					simple_strtol(ap[2], NULL, 0);
 			break;
 		case OP_INT_LE:
-			expr = simple_strtol(ap[0], NULL, 10) <=
-					simple_strtol(ap[2], NULL, 10);
+			expr = simple_strtol(ap[0], NULL, 0) <=
+					simple_strtol(ap[2], NULL, 0);
 			break;
 		case OP_INT_GT:
-			expr = simple_strtol(ap[0], NULL, 10) >
-					simple_strtol(ap[2], NULL, 10);
+			expr = simple_strtol(ap[0], NULL, 0) >
+					simple_strtol(ap[2], NULL, 0);
 			break;
 		case OP_INT_GE:
-			expr = simple_strtol(ap[0], NULL, 10) >=
-					simple_strtol(ap[2], NULL, 10);
+			expr = simple_strtol(ap[0], NULL, 0) >=
+					simple_strtol(ap[2], NULL, 0);
 			break;
 		case OP_FILE_EXISTS:
 			expr = file_exists(ap[1], ap[2], ap[3], FS_TYPE_ANY);