Re: [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.