From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C537441C5DC for ; Thu, 25 Jun 2020 17:21:44 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) From: Chris Sosnin In-Reply-To: Date: Thu, 25 Jun 2020 17:21:43 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <83FCC0CE-3F65-4726-96D4-71E470D5B216@tarantool.org> References: <7e17dc729687c7c61da5f761e4302e6e78b9d01f.1593017012.git.k.sosnin@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/2] Refactor decNumberFromString List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the review! > On 25 Jun 2020, at 01:22, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! >=20 > 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. >=20 >> 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) { >=20 > 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: >=20 > const char * decNumberFromString(decNumber *dn, const char chars[], > decContext *set) { >=20 > Not like this: >=20 > 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. >=20 >> 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); >=20 > 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. >=20 > So as it would be >=20 > decNumber * decNumberFromUInt64(decNumber *, uint64_t); > const char * > decNumberFromString(decNumber *, const char *, = decContext *); > char * decNumberToString(const decNumber *, char *); >=20 > Or break the alignment: >=20 > decNumber * decNumberFromUInt64(decNumber *, uint64_t); > const char * decNumberFromString(decNumber *, const char *, = decContext *); > char * decNumberToString(const decNumber *, char *); >=20 > But I really don't like when alignment causes so many changes. I removed the extra spaces for other lines on the branch.