Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL
@ 2020-06-24 16:53 Chris Sosnin
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString Chris Sosnin
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Chris Sosnin @ 2020-06-24 16:53 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko, v.shpilevoy

Changes in v2:
- Instead of skipping whitespaces, allow FromString to parse
valid beginning of the string and return pointer to the next
character. Thus, we can check for whitespaces ourselves.
This also requires lib/core changes: either pass a pointer
to decimal_from_string and check for '\0' after every use, or 
refactor it so it works the same way and introduce something like strtodec.
- Rename decIsWhole -> decIsInt.

branch: https://github.com/tarantool/decNumber/tree/ksosnin/utilities-for-sql
related issue: https://github.com/tarantool/tarantool/issues/4415

Chris Sosnin (2):
  Refactor decNumberFromString
  Add IsInt method for checking the fractional part of a number

 decNumber.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 decNumber.h | 27 ++++++++++++++-------------
 2 files changed, 52 insertions(+), 21 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString
  2020-06-24 16:53 [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Chris Sosnin
@ 2020-06-24 16:53 ` Chris Sosnin
  2020-06-24 22:22   ` Vladislav Shpilevoy
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number Chris Sosnin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Chris Sosnin @ 2020-06-24 16:53 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko, v.shpilevoy

Allow parsing a valid beginning of the string to unify the behavior
with strto* functions.

Needed for tarantool/tarantool#4415
---
 decNumber.c | 19 +++++++++++--------
 decNumber.h | 26 +++++++++++++-------------
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/decNumber.c b/decNumber.c
index 85deb13..e248656 100644
--- a/decNumber.c
+++ b/decNumber.c
@@ -545,7 +545,7 @@ char * decNumberToEngString(const decNumber *dn, char *string){
 /*                                                                    */
 /* If bad syntax is detected, the result will be a quiet NaN.         */
 /* ------------------------------------------------------------------ */
-decNumber * decNumberFromString(decNumber *dn, const char chars[],
+const char * decNumberFromString(decNumber *dn, const char chars[],
                                 decContext *set) {
   Int   exponent=0;                // working exponent [assume 0]
   uByte bits=0;                    // working flags [assume +ve]
@@ -557,6 +557,7 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
   const char *dotchar=NULL;        // where dot was found
   const char *cfirst=chars;        // -> first character of decimal part
   const char *last=NULL;           // -> last digit of decimal part
+  const char *end=chars;
   const char *c;                   // work
   Unit  *up;                       // ..
   #if DECDPUN>1
@@ -649,17 +650,18 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
 
      else if (*c!='\0') {          // more to process...
       // had some digits; exponent is only valid sequence now
+      status=0;                    // we have a number already
+      end=c;                       // backup for invalid exponent
       Flag nege;                   // 1=negative exponent
       const char *firstexp;        // -> first significant exponent digit
-      status=DEC_Conversion_syntax;// assume the worst
-      if (*c!='e' && *c!='E') break;
+      if (*c!='e' && *c!='E') goto finalize;
       /* Found 'e' or 'E' -- now process explicit exponent */
       // 1998.07.11: sign no longer required
       nege=0;
       c++;                         // to (possible) sign
       if (*c=='-') {nege=1; c++;}
        else if (*c=='+') c++;
-      if (*c=='\0') break;
+      if (*c<'0' || *c>'9') goto finalize;
 
       for (; *c=='0' && *(c+1)!='\0';) c++;  // strip insignificant zeros
       firstexp=c;                            // save exponent digit place
@@ -667,8 +669,8 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
         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;
+
+      end=c;
 
       // (this next test must be after the syntax checks)
       // if it was too long the exponent may have wrapped, so check
@@ -678,9 +680,10 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
         // [up to 1999999999 is OK, for example 1E-1000000998]
         }
       if (nege) exponent=-exponent;     // was negative
-      status=0;                         // is OK
       } // stuff after digits
 
+      else end=c;
+finalize:
     // Here when whole string has been inspected; syntax is good
     // cfirst->first digit (never dot), last->last digit (ditto)
 
@@ -772,7 +775,7 @@ decNumber * decNumberFromString(decNumber *dn, const char chars[],
 
   if (allocres!=NULL) free(allocres);   // drop any storage used
   if (status!=0) decStatus(dn, status, set);
-  return dn;
+  return end;
   } /* decNumberFromString */
 
 /* ================================================================== */
diff --git a/decNumber.h b/decNumber.h
index 85a3f3f..ffaa3d8 100644
--- a/decNumber.h
+++ b/decNumber.h
@@ -101,19 +101,19 @@
   /* decNumber public functions and macros                            */
   /* ---------------------------------------------------------------- */
   /* Conversions                                                      */
-  decNumber * decNumberFromInt32(decNumber *, int32_t);
-  decNumber * decNumberFromUInt32(decNumber *, uint32_t);
-  decNumber * decNumberFromInt64(decNumber *, int64_t);
-  decNumber * decNumberFromUInt64(decNumber *, uint64_t);
-  decNumber * decNumberFromString(decNumber *, const char *, decContext *);
-  char      * decNumberToString(const decNumber *, char *);
-  char      * decNumberToEngString(const decNumber *, char *);
-  uint32_t    decNumberToUInt32(const decNumber *, decContext *);
-  uint64_t    decNumberToUInt64(const decNumber *, decContext *);
-  int32_t     decNumberToInt32(const decNumber *, decContext *);
-  int64_t     decNumberToInt64(const decNumber *, decContext *);
-  uint8_t   * decNumberGetBCD(const decNumber *, uint8_t *);
-  decNumber * decNumberSetBCD(decNumber *, const uint8_t *, uint32_t);
+  decNumber  * decNumberFromInt32(decNumber *, int32_t);
+  decNumber  * decNumberFromUInt32(decNumber *, uint32_t);
+  decNumber  * decNumberFromInt64(decNumber *, int64_t);
+  decNumber  * decNumberFromUInt64(decNumber *, uint64_t);
+  const char * decNumberFromString(decNumber *, const char *, decContext *);
+  char       * decNumberToString(const decNumber *, char *);
+  char       * decNumberToEngString(const decNumber *, char *);
+  uint32_t     decNumberToUInt32(const decNumber *, decContext *);
+  uint64_t     decNumberToUInt64(const decNumber *, decContext *);
+  int32_t      decNumberToInt32(const decNumber *, decContext *);
+  int64_t      decNumberToInt64(const decNumber *, decContext *);
+  uint8_t    * decNumberGetBCD(const decNumber *, uint8_t *);
+  decNumber  * decNumberSetBCD(decNumber *, const uint8_t *, uint32_t);
 
   /* Operators and elementary functions                               */
   decNumber * decNumberAbs(decNumber *, const decNumber *, decContext *);
-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number
  2020-06-24 16:53 [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Chris Sosnin
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString Chris Sosnin
@ 2020-06-24 16:53 ` Chris Sosnin
  2020-06-24 22:23   ` Vladislav Shpilevoy
  2020-06-25 21:04 ` [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Vladislav Shpilevoy
  2020-06-26 10:57 ` Serge Petrenko
  3 siblings, 1 reply; 9+ messages in thread
From: Chris Sosnin @ 2020-06-24 16:53 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko, v.shpilevoy

Currently there is no efficient way to do this.

Needed for tarantool/tarantool#4415
---
 decNumber.c | 27 +++++++++++++++++++++++++++
 decNumber.h |  1 +
 2 files changed, 28 insertions(+)

diff --git a/decNumber.c b/decNumber.c
index e248656..26acec3 100644
--- a/decNumber.c
+++ b/decNumber.c
@@ -501,6 +501,33 @@ uLong decNumberToUInt64(const decNumber *dn, decContext *set) {
   return 0;
   }  // decNumberToUInt64
 
+Flag decNumberIsInt(const decNumber *dn) {
+  const Unit *up=dn->lsu;
+  if (dn->exponent>=0) {
+    return 1;
+    }
+   else {
+    Int count=-dn->exponent;
+    // spin up whole units until reach the Unit with the unit digit
+    for (; count>=DECDPUN; up++) {
+      if (*up!=0) return 0;
+      count-=DECDPUN;
+      }
+    if (count==0) return 1;             // [a multiple of DECDPUN]
+     else {                             // [not multiple of DECDPUN]
+      Int rem;                          // work
+      // slice off fraction digits and check for non-zero
+      #if DECDPUN<=4
+        rem=*up-QUOT10(*up, count)*powers[count];
+      #else
+        rem=*up%powers[count];          // slice off discards
+      #endif
+      if (rem!=0) return 0;
+      }
+    }
+    return 1;
+  }  // decNumberIsInt
+
 /* ------------------------------------------------------------------ */
 /* to-scientific-string -- conversion to numeric string               */
 /* to-engineering-string -- conversion to numeric string              */
diff --git a/decNumber.h b/decNumber.h
index ffaa3d8..9d3a7e1 100644
--- a/decNumber.h
+++ b/decNumber.h
@@ -169,6 +169,7 @@
   decNumber  * decNumberTrim(decNumber *);
   const char * decNumberVersion(void);
   decNumber  * decNumberZero(decNumber *);
+  uint8_t      decNumberIsInt(const decNumber *dn);
 
   /* Functions for testing decNumbers (normality depends on context)  */
   int32_t decNumberIsNormal(const decNumber *, decContext *);
-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString Chris Sosnin
@ 2020-06-24 22:22   ` Vladislav Shpilevoy
  2020-06-25 14:21     ` Chris Sosnin
  0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-24 22:22 UTC (permalink / raw)
  To: Chris Sosnin, tarantool-patches, sergepetrenko

Hi! Thanks for the patch!

Technically looks fine, although I didn't test it except for
the tests we already have. You probably should create a patchset
for the main repository with decnumber version bump + decimal.h
API update + unit tests in unit/decimal.c. Before this commit
is merged, so as you could fix any potential errors.

> diff --git a/decNumber.c b/decNumber.c
> index 85deb13..e248656 100644
> --- a/decNumber.c
> +++ b/decNumber.c
> @@ -545,7 +545,7 @@ char * decNumberToEngString(const decNumber *dn, char *string){
>  /*                                                                    */
>  /* If bad syntax is detected, the result will be a quiet NaN.         */
>  /* ------------------------------------------------------------------ */
> -decNumber * decNumberFromString(decNumber *dn, const char chars[],
> +const char * decNumberFromString(decNumber *dn, const char chars[],
>                                  decContext *set) {

1. The code style is absolutely dead, but it is consistently dead.
Part of its consistency was the multi-line argument alignment, the
same as in Tarantool. Here the second line should be aligned under
the first argument of the first line:

    const char * decNumberFromString(decNumber *dn, const char chars[],
                                     decContext *set) {

Not like this:

    const char * decNumberFromString(decNumber *dn, const char chars[],
                                    decContext *set) {

> diff --git a/decNumber.h b/decNumber.h
> index 85a3f3f..ffaa3d8 100644
> --- a/decNumber.h
> +++ b/decNumber.h
> @@ -101,19 +101,19 @@
>    /* decNumber public functions and macros                            */
>    /* ---------------------------------------------------------------- */
>    /* Conversions                                                      */
> -  decNumber * decNumberFromInt32(decNumber *, int32_t);
> -  decNumber * decNumberFromUInt32(decNumber *, uint32_t);
> -  decNumber * decNumberFromInt64(decNumber *, int64_t);
> -  decNumber * decNumberFromUInt64(decNumber *, uint64_t);
> -  decNumber * decNumberFromString(decNumber *, const char *, decContext *);
> -  char      * decNumberToString(const decNumber *, char *);
> -  char      * decNumberToEngString(const decNumber *, char *);
> -  uint32_t    decNumberToUInt32(const decNumber *, decContext *);
> -  uint64_t    decNumberToUInt64(const decNumber *, decContext *);
> -  int32_t     decNumberToInt32(const decNumber *, decContext *);
> -  int64_t     decNumberToInt64(const decNumber *, decContext *);
> -  uint8_t   * decNumberGetBCD(const decNumber *, uint8_t *);
> -  decNumber * decNumberSetBCD(decNumber *, const uint8_t *, uint32_t);
> +  decNumber  * decNumberFromInt32(decNumber *, int32_t);
> +  decNumber  * decNumberFromUInt32(decNumber *, uint32_t);
> +  decNumber  * decNumberFromInt64(decNumber *, int64_t);
> +  decNumber  * decNumberFromUInt64(decNumber *, uint64_t);
> +  const char * decNumberFromString(decNumber *, const char *, decContext *);
> +  char       * decNumberToString(const decNumber *, char *);
> +  char       * decNumberToEngString(const decNumber *, char *);
> +  uint32_t     decNumberToUInt32(const decNumber *, decContext *);
> +  uint64_t     decNumberToUInt64(const decNumber *, decContext *);
> +  int32_t      decNumberToInt32(const decNumber *, decContext *);
> +  int64_t      decNumberToInt64(const decNumber *, decContext *);
> +  uint8_t    * decNumberGetBCD(const decNumber *, uint8_t *);
> +  decNumber  * decNumberSetBCD(decNumber *, const uint8_t *, uint32_t);

2. Lets not do so many changes. I think it is ok to break
the alignment in one line. Or you can wrap function name on
a next line, if you want.

So as it would be

    decNumber * decNumberFromUInt64(decNumber *, uint64_t);
    const char *
                decNumberFromString(decNumber *, const char *, decContext *);
    char      * decNumberToString(const decNumber *, char *);

Or break the alignment:

    decNumber * decNumberFromUInt64(decNumber *, uint64_t);
    const char * decNumberFromString(decNumber *, const char *, decContext *);
    char      * decNumberToString(const decNumber *, char *);

But I really don't like when alignment causes so many changes.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number Chris Sosnin
@ 2020-06-24 22:23   ` Vladislav Shpilevoy
  2020-06-25 14:22     ` Chris Sosnin
  0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-24 22:23 UTC (permalink / raw)
  To: Chris Sosnin, tarantool-patches, sergepetrenko

Thanks for the patch!

Technically looks fine. But see two style comments below.

On 24/06/2020 18:53, Chris Sosnin wrote:
> Currently there is no efficient way to do this.
> 
> Needed for tarantool/tarantool#4415
> ---
>  decNumber.c | 27 +++++++++++++++++++++++++++
>  decNumber.h |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/decNumber.c b/decNumber.c
> index e248656..26acec3 100644
> --- a/decNumber.c
> +++ b/decNumber.c
> @@ -501,6 +501,33 @@ uLong decNumberToUInt64(const decNumber *dn, decContext *set) {
>    return 0;
>    }  // decNumberToUInt64
>  
> +Flag decNumberIsInt(const decNumber *dn) {

1. It seems Flag type is internal. For public API flags the lib uses int32_t.
For example, look at decNumberIsNormal(), decNumberIsSubnormal().

> +  const Unit *up=dn->lsu;
> +  if (dn->exponent>=0) {
> +    return 1;
> +    }
> +   else {
> +    Int count=-dn->exponent;
> +    // spin up whole units until reach the Unit with the unit digit
> +    for (; count>=DECDPUN; up++) {
> +      if (*up!=0) return 0;
> +      count-=DECDPUN;
> +      }
> +    if (count==0) return 1;             // [a multiple of DECDPUN]
> +     else {                             // [not multiple of DECDPUN]
> +      Int rem;                          // work
> +      // slice off fraction digits and check for non-zero
> +      #if DECDPUN<=4
> +        rem=*up-QUOT10(*up, count)*powers[count];
> +      #else
> +        rem=*up%powers[count];          // slice off discards
> +      #endif
> +      if (rem!=0) return 0;
> +      }
> +    }
> +    return 1;
> +  }  // decNumberIsInt
> +
>  /* ------------------------------------------------------------------ */
>  /* to-scientific-string -- conversion to numeric string               */
>  /* to-engineering-string -- conversion to numeric string              */
> diff --git a/decNumber.h b/decNumber.h
> index ffaa3d8..9d3a7e1 100644
> --- a/decNumber.h
> +++ b/decNumber.h
> @@ -169,6 +169,7 @@
>    decNumber  * decNumberTrim(decNumber *);
>    const char * decNumberVersion(void);
>    decNumber  * decNumberZero(decNumber *);
> +  uint8_t      decNumberIsInt(const decNumber *dn);

2. Better move this to the other Is functions a few lines below. Where
the comment says "Functions for testing decNumbers".

>  
>    /* Functions for testing decNumbers (normality depends on context)  */
>    int32_t decNumberIsNormal(const decNumber *, decContext *);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString
  2020-06-24 22:22   ` Vladislav Shpilevoy
@ 2020-06-25 14:21     ` Chris Sosnin
  0 siblings, 0 replies; 9+ messages in thread
From: Chris Sosnin @ 2020-06-25 14:21 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review!

> On 25 Jun 2020, at 01:22, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
> Technically looks fine, although I didn't test it except for
> the tests we already have. You probably should create a patchset
> for the main repository with decnumber version bump + decimal.h
> API update + unit tests in unit/decimal.c. Before this commit
> is merged, so as you could fix any potential errors.

Done. I will send it as a separate patchset. Bumped decNumber library is with fixed codestyle.

> 
>> diff --git a/decNumber.c b/decNumber.c
>> index 85deb13..e248656 100644
>> --- a/decNumber.c
>> +++ b/decNumber.c
>> @@ -545,7 +545,7 @@ char * decNumberToEngString(const decNumber *dn, char *string){
>> /*                                                                    */
>> /* If bad syntax is detected, the result will be a quiet NaN.         */
>> /* ------------------------------------------------------------------ */
>> -decNumber * decNumberFromString(decNumber *dn, const char chars[],
>> +const char * decNumberFromString(decNumber *dn, const char chars[],
>>                                 decContext *set) {
> 
> 1. The code style is absolutely dead, but it is consistently dead.
> Part of its consistency was the multi-line argument alignment, the
> same as in Tarantool. Here the second line should be aligned under
> the first argument of the first line:
> 
>    const char * decNumberFromString(decNumber *dn, const char chars[],
>                                     decContext *set) {
> 
> Not like this:
> 
>    const char * decNumberFromString(decNumber *dn, const char chars[],
>                                    decContext *set) {

I agree, I forgot to update the indentation along with the type. Fixed on branch.

> 
>> diff --git a/decNumber.h b/decNumber.h
>> index 85a3f3f..ffaa3d8 100644
>> --- a/decNumber.h
>> +++ b/decNumber.h
>> @@ -101,19 +101,19 @@
>>   /* decNumber public functions and macros                            */
>>   /* ---------------------------------------------------------------- */
>>   /* Conversions                                                      */
>> -  decNumber * decNumberFromInt32(decNumber *, int32_t);
>> -  decNumber * decNumberFromUInt32(decNumber *, uint32_t);
>> -  decNumber * decNumberFromInt64(decNumber *, int64_t);
>> -  decNumber * decNumberFromUInt64(decNumber *, uint64_t);
>> -  decNumber * decNumberFromString(decNumber *, const char *, decContext *);
>> -  char      * decNumberToString(const decNumber *, char *);
>> -  char      * decNumberToEngString(const decNumber *, char *);
>> -  uint32_t    decNumberToUInt32(const decNumber *, decContext *);
>> -  uint64_t    decNumberToUInt64(const decNumber *, decContext *);
>> -  int32_t     decNumberToInt32(const decNumber *, decContext *);
>> -  int64_t     decNumberToInt64(const decNumber *, decContext *);
>> -  uint8_t   * decNumberGetBCD(const decNumber *, uint8_t *);
>> -  decNumber * decNumberSetBCD(decNumber *, const uint8_t *, uint32_t);
>> +  decNumber  * decNumberFromInt32(decNumber *, int32_t);
>> +  decNumber  * decNumberFromUInt32(decNumber *, uint32_t);
>> +  decNumber  * decNumberFromInt64(decNumber *, int64_t);
>> +  decNumber  * decNumberFromUInt64(decNumber *, uint64_t);
>> +  const char * decNumberFromString(decNumber *, const char *, decContext *);
>> +  char       * decNumberToString(const decNumber *, char *);
>> +  char       * decNumberToEngString(const decNumber *, char *);
>> +  uint32_t     decNumberToUInt32(const decNumber *, decContext *);
>> +  uint64_t     decNumberToUInt64(const decNumber *, decContext *);
>> +  int32_t      decNumberToInt32(const decNumber *, decContext *);
>> +  int64_t      decNumberToInt64(const decNumber *, decContext *);
>> +  uint8_t    * decNumberGetBCD(const decNumber *, uint8_t *);
>> +  decNumber  * decNumberSetBCD(decNumber *, const uint8_t *, uint32_t);
> 
> 2. Lets not do so many changes. I think it is ok to break
> the alignment in one line. Or you can wrap function name on
> a next line, if you want.
> 
> So as it would be
> 
>    decNumber * decNumberFromUInt64(decNumber *, uint64_t);
>    const char *
>                decNumberFromString(decNumber *, const char *, decContext *);
>    char      * decNumberToString(const decNumber *, char *);
> 
> Or break the alignment:
> 
>    decNumber * decNumberFromUInt64(decNumber *, uint64_t);
>    const char * decNumberFromString(decNumber *, const char *, decContext *);
>    char      * decNumberToString(const decNumber *, char *);
> 
> But I really don't like when alignment causes so many changes.

I removed the extra spaces for other lines on the branch.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number
  2020-06-24 22:23   ` Vladislav Shpilevoy
@ 2020-06-25 14:22     ` Chris Sosnin
  0 siblings, 0 replies; 9+ messages in thread
From: Chris Sosnin @ 2020-06-25 14:22 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review!

> On 25 Jun 2020, at 01:23, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the patch!
> 
> Technically looks fine. But see two style comments below.
> 
> On 24/06/2020 18:53, Chris Sosnin wrote:
>> Currently there is no efficient way to do this.
>> 
>> Needed for tarantool/tarantool#4415
>> ---
>> decNumber.c | 27 +++++++++++++++++++++++++++
>> decNumber.h |  1 +
>> 2 files changed, 28 insertions(+)
>> 
>> diff --git a/decNumber.c b/decNumber.c
>> index e248656..26acec3 100644
>> --- a/decNumber.c
>> +++ b/decNumber.c
>> @@ -501,6 +501,33 @@ uLong decNumberToUInt64(const decNumber *dn, decContext *set) {
>>   return 0;
>>   }  // decNumberToUInt64
>> 
>> +Flag decNumberIsInt(const decNumber *dn) {
> 
> 1. It seems Flag type is internal. For public API flags the lib uses int32_t.
> For example, look at decNumberIsNormal(), decNumberIsSubnormal().

I agree, Flag is only used inside decNumber.c, I looked at the wrong function.
Fixed.

> 
>> +  const Unit *up=dn->lsu;
>> +  if (dn->exponent>=0) {
>> +    return 1;
>> +    }
>> +   else {
>> +    Int count=-dn->exponent;
>> +    // spin up whole units until reach the Unit with the unit digit
>> +    for (; count>=DECDPUN; up++) {
>> +      if (*up!=0) return 0;
>> +      count-=DECDPUN;
>> +      }
>> +    if (count==0) return 1;             // [a multiple of DECDPUN]
>> +     else {                             // [not multiple of DECDPUN]
>> +      Int rem;                          // work
>> +      // slice off fraction digits and check for non-zero
>> +      #if DECDPUN<=4
>> +        rem=*up-QUOT10(*up, count)*powers[count];
>> +      #else
>> +        rem=*up%powers[count];          // slice off discards
>> +      #endif
>> +      if (rem!=0) return 0;
>> +      }
>> +    }
>> +    return 1;
>> +  }  // decNumberIsInt
>> +
>> /* ------------------------------------------------------------------ */
>> /* to-scientific-string -- conversion to numeric string               */
>> /* to-engineering-string -- conversion to numeric string              */
>> diff --git a/decNumber.h b/decNumber.h
>> index ffaa3d8..9d3a7e1 100644
>> --- a/decNumber.h
>> +++ b/decNumber.h
>> @@ -169,6 +169,7 @@
>>   decNumber  * decNumberTrim(decNumber *);
>>   const char * decNumberVersion(void);
>>   decNumber  * decNumberZero(decNumber *);
>> +  uint8_t      decNumberIsInt(const decNumber *dn);
> 
> 2. Better move this to the other Is functions a few lines below. Where
> the comment says "Functions for testing decNumbers".

Fixed.

> 
>> 
>>   /* Functions for testing decNumbers (normality depends on context)  */
>>   int32_t decNumberIsNormal(const decNumber *, decContext *);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL
  2020-06-24 16:53 [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Chris Sosnin
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString Chris Sosnin
  2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number Chris Sosnin
@ 2020-06-25 21:04 ` Vladislav Shpilevoy
  2020-06-26 10:57 ` Serge Petrenko
  3 siblings, 0 replies; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-25 21:04 UTC (permalink / raw)
  To: Chris Sosnin, tarantool-patches, sergepetrenko

LGTM. But better add tests and a wrapper for decNumberIsInt()
in the tarantool/tarantool's patch.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL
  2020-06-24 16:53 [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Chris Sosnin
                   ` (2 preceding siblings ...)
  2020-06-25 21:04 ` [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Vladislav Shpilevoy
@ 2020-06-26 10:57 ` Serge Petrenko
  3 siblings, 0 replies; 9+ messages in thread
From: Serge Petrenko @ 2020-06-26 10:57 UTC (permalink / raw)
  To: Chris Sosnin, tarantool-patches, v.shpilevoy


24.06.2020 19:53, Chris Sosnin пишет:
> Changes in v2:
> - Instead of skipping whitespaces, allow FromString to parse
> valid beginning of the string and return pointer to the next
> character. Thus, we can check for whitespaces ourselves.
> This also requires lib/core changes: either pass a pointer
> to decimal_from_string and check for '\0' after every use, or
> refactor it so it works the same way and introduce something like strtodec.
> - Rename decIsWhole -> decIsInt.
>
> branch: https://github.com/tarantool/decNumber/tree/ksosnin/utilities-for-sql
> related issue: https://github.com/tarantool/tarantool/issues/4415


LGTM

>
> Chris Sosnin (2):
>    Refactor decNumberFromString
>    Add IsInt method for checking the fractional part of a number
>
>   decNumber.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>   decNumber.h | 27 ++++++++++++++-------------
>   2 files changed, 52 insertions(+), 21 deletions(-)
>
-- 
Serge Petrenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-06-26 10:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-24 16:53 [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Chris Sosnin
2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString Chris Sosnin
2020-06-24 22:22   ` Vladislav Shpilevoy
2020-06-25 14:21     ` Chris Sosnin
2020-06-24 16:53 ` [Tarantool-patches] [PATCH v2 2/2] Add IsInt method for checking the fractional part of a number Chris Sosnin
2020-06-24 22:23   ` Vladislav Shpilevoy
2020-06-25 14:22     ` Chris Sosnin
2020-06-25 21:04 ` [Tarantool-patches] [PATCH v2 0/2] decNumber utilites for SQL Vladislav Shpilevoy
2020-06-26 10:57 ` Serge Petrenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox