Re: [Tarantool-patches] [PATCH 1/2] Allow leading and trailing whitespaces in FromString

[Chris Sosnin <k.sosnin@tarantool.org>]

Hi! Thank you for the review!

> On 6 Jun 2020, at 16:48, Serge Petrenko <sergepetrenko@tarantool.org> wrote:
> 
> 
> 05.06.2020 18:41, Chris Sosnin пишет:
>> This is needed for SQL, since these whitespaces are allowed in
>> conversions from string.
>> 
>> Needed for tarantool/tarantool#4415
> 
> Hi! Thanks for the patch!
> 
> See one comment below.
> 
>> ---
>>  decNumber.c | 44 +++++++++++++++++++++++++++++---------------
>>  1 file changed, 29 insertions(+), 15 deletions(-)
>> 
>> diff --git a/decNumber.c b/decNumber.c
>> index 85deb13..7c53bc3 100644
>> --- a/decNumber.c
>> +++ b/decNumber.c
>> @@ -555,9 +555,9 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>    Unit  *allocres=NULL;            // -> allocated result, iff allocated
>>    Int   d=0;                       // count of digits found in decimal part
>>    const char *dotchar=NULL;        // where dot was found
>> -  const char *cfirst=chars;        // -> first character of decimal part
>> +  const char *cfirst=NULL;         // -> first character of decimal part
>>    const char *last=NULL;           // -> last digit of decimal part
>> -  const char *c;                   // work
>> +  const char *c, *p;               // work
>>    Unit  *up;                       // ..
>>    #if DECDPUN>1
>>    Int   cut, out;                  // ..
>> @@ -571,6 +571,9 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>    #endif
>>      do {                             // status & malloc protection
>> +    for (; *chars==' '; ++chars);  // skip leading whitespaces
>> +    cfirst=chars;
>> +
>>      for (c=chars;; c++) {          // -> input character
>>        if (*c>='0' && *c<='9') {    // test for Arabic digit
>>          last=c;
>> @@ -590,13 +593,14 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>            cfirst++;
>>            continue;}
>>          }
>> +      for (p=c; *p==' '; ++p);
> Looks like this `for` loop should go below.
>>        // *c is not a digit, or a valid +, -, or '.'
>>        break;
>>        } // c
>>  
> 
> Here, ^ (This guy's formatting is confusing as hell, I know)

You are right, thank you for the catch!
I applied the change on the branch.

> 
>>      if (last==NULL) {              // no digits yet
>>        status=DEC_Conversion_syntax;// assume the worst
>> -      if (*c=='\0') break;         // and no more to come...
>> +      if (*p=='\0') break;         // and no more to come...
>>        #if DECSUBSET
>>        // if subset then infinities and NaNs are not allowed
>>        if (!set->extended) break;   // hopeless
>> @@ -626,7 +630,8 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>        // now either nothing, or nnnn payload, expected
>>        // -> start of integer and skip leading 0s [including plain 0]
>>        for (cfirst=c; *cfirst=='0';) cfirst++;
>> -      if (*cfirst=='\0') {         // "NaN" or "sNaN", maybe with all 0s
>> +      for (p=cfirst; *p==' '; ++p);
>> +      if (*p=='\0') {              // "NaN" or "sNaN", maybe with all 0s
>>          status=0;                  // it's good
>>          break;                     // ..
>>          }
>> @@ -635,7 +640,8 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>          if (*c<'0' || *c>'9') break; // test for Arabic digit
>>          last=c;
>>          }
>> -      if (*c!='\0') break;         // not all digits
>> +      for (p=c; *p==' '; ++p);
>> +      if (*p!='\0') break;         // not all digits
>>        if (d>set->digits-1) {
>>          // [NB: payload in a decNumber can be full length unless
>>          // clamped, in which case can only be digits-1]
>> @@ -647,7 +653,7 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>        bits=dn->bits;               // for copy-back
>>        } // last==NULL
>>  -     else if (*c!='\0') {          // more to process...
>> +     else if (p==c && *p!='\0') {  // more to process...
>>        // had some digits; exponent is only valid sequence now
>>        Flag nege;                   // 1=negative exponent
>>        const char *firstexp;        // -> first significant exponent digit
>> @@ -659,16 +665,18 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>        c++;                         // to (possible) sign
>>        if (*c=='-') {nege=1; c++;}
>>         else if (*c=='+') c++;
>> -      if (*c=='\0') break;
>> -
>> -      for (; *c=='0' && *(c+1)!='\0';) c++;  // strip insignificant zeros
>> +      for (p=c; *p==' '; ++p);
>> +      if (*p=='\0') break;
>> +      // strip insignificant zeros
>> +      for (; *c=='0' && *(c+1)!='\0' && *(c+1)!=' ';) c++;
>>        firstexp=c;                            // save exponent digit place
>>        for (; ;c++) {
>>          if (*c<'0' || *c>'9') break;         // not a digit
>>          exponent=X10(exponent)+(Int)*c-(Int)'0';
>>          } // c
>>        // if not now on a '\0', *c must not be a digit
>> -      if (*c!='\0') break;
>> +      for (p=c; *p==' '; ++p);
>> +      if (*p!='\0') break;
>>          // (this next test must be after the syntax checks)
>>        // if it was too long the exponent may have wrapped, so check
>> @@ -681,6 +689,11 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
>>        status=0;                         // is OK
>>        } // stuff after digits
>>  +     else if (*p!='\0') {    // whitespaces in the middle
>> +       status=DEC_Conversion_syntax;
>> +       break;
>> +     }
>> +
>>      // Here when whole string has been inspected; syntax is good
>>      // cfirst->first digit (never dot), last->last digit (ditto)
>>  @@ -7739,12 +7752,13 @@ static decNumber *decDecap(decNumber *dn, Int drop) {
>>  /*   if (decBiStr(test, "mike", "MIKE")) ...                          */
>>  /* ------------------------------------------------------------------ */
>>  static Flag decBiStr(const char *targ, const char *str1, const char *str2) {
>> -  for (;;targ++, str1++, str2++) {
>> +  const char *end=strchr(targ, ' ');
>> +  if (end==NULL)
>> +    end=targ+strlen(targ);
>> +  for (;targ<end; targ++, str1++, str2++) {
>>      if (*targ!=*str1 && *targ!=*str2) return 0;
>> -    // *targ has a match in one (or both, if terminator)
>> -    if (*targ=='\0') break;
>> -    } // forever
>> -  return 1;
>> +    } // for
>> +  return *str1=='\0';
>>    } // decBiStr
>>    /* ------------------------------------------------------------------ */
> 
> -- 
> Serge Petrenko