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

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

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.