From: Artem <artemreyt@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>,
Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10
Date: Wed, 25 Nov 2020 12:52:22 +0300 [thread overview]
Message-ID: <4ee13a26-9079-9a3e-9bb9-68854fef6c43@tarantool.org> (raw)
In-Reply-To: <20201123231440.GA17397@tarantool.org>
Nikita, thanks for review!
I considered your proposal to use stack `struct Select` variable and
make `memcpy` after filling that varible. It really looks better.
I only changed naming (because, pNew means pointer
and i had to sacrifice diff for better readability).
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index b0554a172..5d4b2f624 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -154,56 +154,52 @@ sqlSelectNew(Parse * pParse, /* Parsing
context */
Expr * pLimit, /* LIMIT value. NULL means not
used */
Expr * pOffset) /* OFFSET value. NULL means no
offset */
{
- Select *pNew;
Select standin;
sql *db = pParse->db;
- pNew = sqlDbMallocRawNN(db, sizeof(*pNew));
- if (pNew == 0) {
- assert(db->mallocFailed);
- pNew = &standin;
- }
if (pEList == 0) {
struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);
if (expr == NULL)
pParse->is_aborted = true;
pEList = sql_expr_list_append(db, NULL, expr);
}
- pNew->pEList = pEList;
- pNew->op = TK_SELECT;
- pNew->selFlags = selFlags;
- pNew->iLimit = 0;
- pNew->iOffset = 0;
+ standin.pEList = pEList;
+ standin.op = TK_SELECT;
+ standin.selFlags = selFlags;
+ standin.iLimit = 0;
+ standin.iOffset = 0;
#ifdef SQL_DEBUG
- pNew->zSelName[0] = 0;
+ standin.zSelName[0] = 0;
if ((pParse->sql_flags & SQL_SelectTrace) != 0)
sqlSelectTrace = 0xfff;
else
sqlSelectTrace = 0;
#endif
- pNew->addrOpenEphm[0] = -1;
- pNew->addrOpenEphm[1] = -1;
- pNew->nSelectRow = 0;
+ standin.addrOpenEphm[0] = -1;
+ standin.addrOpenEphm[1] = -1;
+ standin.nSelectRow = 0;
if (pSrc == 0)
pSrc = sqlDbMallocZero(db, sizeof(*pSrc));
- pNew->pSrc = pSrc;
- pNew->pWhere = pWhere;
- pNew->pGroupBy = pGroupBy;
- pNew->pHaving = pHaving;
- pNew->pOrderBy = pOrderBy;
- pNew->pPrior = 0;
- pNew->pNext = 0;
- pNew->pLimit = pLimit;
- pNew->pOffset = pOffset;
- pNew->pWith = 0;
+ standin.pSrc = pSrc;
+ standin.pWhere = pWhere;
+ standin.pGroupBy = pGroupBy;
+ standin.pHaving = pHaving;
+ standin.pOrderBy = pOrderBy;
+ standin.pPrior = 0;
+ standin.pNext = 0;
+ standin.pLimit = pLimit;
+ standin.pOffset = pOffset;
+ standin.pWith = 0;
assert(pOffset == 0 || pLimit != 0 || pParse->is_aborted
|| db->mallocFailed != 0);
+ Select *pNew = sqlDbMallocRawNN(db, sizeof(*pNew));
if (db->mallocFailed) {
- clearSelect(db, pNew, pNew != &standin);
- pNew = 0;
- } else {
- assert(pNew->pSrc != 0 || pParse->is_aborted);
+ clearSelect(db, &standin, 0);
+ if (pNew != NULL)
+ sqlDbFree(db, pNew);
+ return NULL;
}
- assert(pNew != &standin);
+ assert(standin.pSrc != 0 || pParse->is_aborted);
+ memcpy(pNew, &standin, sizeof(standin));
return pNew;
}
24.11.2020 02:14, Nikita Pettik пишет:
> On 20 Nov 05:04, Artem Starshov wrote:
>> GCC 10 produces the following error:
>> cc1: warning: function may return address of local variable [-Wreturn-local-addr]
>>
>> Fix it.
>>
>> Part-of #4966
>> ---
>> src/box/sql/select.c | 54 +++++++++++++++++++++++---------------------
>> 1 file changed, 28 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index b0554a172..921fab7fc 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -154,13 +154,15 @@ sqlSelectNew(Parse * pParse, /* Parsing context */
>> Expr * pLimit, /* LIMIT value. NULL means not used */
>> Expr * pOffset) /* OFFSET value. NULL means no offset */
>> {
>> - Select *pNew;
>> - Select standin;
>> + Select *pNew; /* Pointer to allocated region by sqlDbMallocRawNN (0 if failed) */
>> + Select *pNewTmp; /* Pointer to work with */
>> + Select standin; /* If allocation failed, save it to pNewTmp for filling and futher cleaning */
> Unfortunatelly, part of SQL codebase is bad formatted. We strive to
> follow original Tarantool codestyle when adding new code chunks.
> So according to comments should not be placed on the lines containing code.
>
> /* comments */
> int x = 3;
> /*comments */
> if (x == 3) {
> ...
>
>> sql *db = pParse->db;
>> pNew = sqlDbMallocRawNN(db, sizeof(*pNew));
>> - if (pNew == 0) {
>> + pNewTmp = pNew;
>> + if (pNewTmp == 0) {
>> assert(db->mallocFailed);
>> - pNew = &standin;
>> + pNewTmp = &standin;
>> }
>> if (pEList == 0) {
>> struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);
>> @@ -168,11 +170,11 @@ sqlSelectNew(Parse * pParse, /* Parsing context */
>> pParse->is_aborted = true;
>> pEList = sql_expr_list_append(db, NULL, expr);
>> }
>> - pNew->pEList = pEList;
>> - pNew->op = TK_SELECT;
>> - pNew->selFlags = selFlags;
>> - pNew->iLimit = 0;
>> - pNew->iOffset = 0;
>> + pNewTmp->pEList = pEList;
>> + pNewTmp->op = TK_SELECT;
>> + pNewTmp->selFlags = selFlags;
>> + pNewTmp->iLimit = 0;
>> + pNewTmp->iOffset = 0;
> Hm, why did you rename pNew -> pNewTmp instead of leaving pNew as is
> and introduce new pointer which is returned from func? Sort of:
>
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 4b069addb..f4806a806 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -154,14 +154,15 @@ sqlSelectNew(Parse * pParse, /* Parsing context */
> Expr * pLimit, /* LIMIT value. NULL means not used */
> Expr * pOffset) /* OFFSET value. NULL means no offset */
> {
> - Select *pNew;
> + Select *pNew, pNewTmp;
> Select standin;
> sql *db = pParse->db;
> - pNew = sqlDbMallocRawNN(db, sizeof(*pNew));
> - if (pNew == 0) {
> + pNewTmp = sqlDbMallocRawNN(db, sizeof(*pNew));
> + if (pNewTmp == 0) {
> assert(db->mallocFailed);
> - pNew = &standin;
> + pNewTmp = &standin;
> }
> + pNew = pNewTmp;
> if (pEList == 0) {
> struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);
> if (expr == NULL)
>
> I mean it would be nice to avoid unnecessary diff.
>
> Alternatively, consider this refactoring:
>
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 4b069addb..5c829c932 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -154,14 +154,8 @@ sqlSelectNew(Parse * pParse, /* Parsing context */
> Expr * pLimit, /* LIMIT value. NULL means not used */
> Expr * pOffset) /* OFFSET value. NULL means no offset */
> {
> - Select *pNew;
> - Select standin;
> + Select pNew;
> sql *db = pParse->db;
> - pNew = sqlDbMallocRawNN(db, sizeof(*pNew));
> - if (pNew == 0) {
> - assert(db->mallocFailed);
> - pNew = &standin;
> - }
> if (pEList == 0) {
> struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);
> if (expr == NULL)
> @@ -197,13 +191,15 @@ sqlSelectNew(Parse * pParse, /* Parsing context */
> pNew->pWith = 0;
> assert(pOffset == 0 || pLimit != 0 || pParse->is_aborted
> || db->mallocFailed != 0);
> + Select *select = sqlDbMallocRawNN(db, sizeof(*pNew));
> if (db->mallocFailed) {
> - clearSelect(db, pNew, pNew != &standin);
> - pNew = 0;
> - } else {
> - assert(pNew->pSrc != 0 || pParse->is_aborted);
> + clearSelect(db, &pNew, 0);
> + if (select != NULL)
> + sqlDbFree(db, select);
> + return NULL;
> }
> - assert(pNew != &standin);
> + assert(pNew->pSrc != 0 || pParse->is_aborted);
> + memcpy(select, &pNew, sizeof(pNew));
> return pNew;
> }
>
> Here we fill in struct Select allocated on stack and in case of
> sqlDbMallocZero/sqlDbMallocRawNN fail we simply don't copy it
> on the structure allocated on heap. IMHO it looks better and will
> work even if the next GCC version will contain more advanced pointer
> aliasing machinery.
>
>> #ifdef SQL_DEBUG
>> pNew->zSelName[0] = 0;
>> if ((pParse->sql_flags & SQL_SelectTrace) != 0)
>> @@ -180,30 +182,30 @@ sqlSelectNew(Parse * pParse, /* Parsing context */
>> else
>> sqlSelectTrace = 0;
>> #endif
>> - pNew->addrOpenEphm[0] = -1;
>> - pNew->addrOpenEphm[1] = -1;
>> - pNew->nSelectRow = 0;
>> + pNewTmp->addrOpenEphm[0] = -1;
>> + pNewTmp->addrOpenEphm[1] = -1;
>> + pNewTmp->nSelectRow = 0;
>> if (pSrc == 0)
>> pSrc = sqlDbMallocZero(db, sizeof(*pSrc));
>> - pNew->pSrc = pSrc;
>> - pNew->pWhere = pWhere;
>> - pNew->pGroupBy = pGroupBy;
>> - pNew->pHaving = pHaving;
>> - pNew->pOrderBy = pOrderBy;
>> - pNew->pPrior = 0;
>> - pNew->pNext = 0;
>> - pNew->pLimit = pLimit;
>> - pNew->pOffset = pOffset;
>> - pNew->pWith = 0;
>> + pNewTmp->pSrc = pSrc;
>> + pNewTmp->pWhere = pWhere;
>> + pNewTmp->pGroupBy = pGroupBy;
>> + pNewTmp->pHaving = pHaving;
>> + pNewTmp->pOrderBy = pOrderBy;
>> + pNewTmp->pPrior = 0;
>> + pNewTmp->pNext = 0;
>> + pNewTmp->pLimit = pLimit;
>> + pNewTmp->pOffset = pOffset;
>> + pNewTmp->pWith = 0;
>> assert(pOffset == 0 || pLimit != 0 || pParse->is_aborted
>> || db->mallocFailed != 0);
>> if (db->mallocFailed) {
>> - clearSelect(db, pNew, pNew != &standin);
>> - pNew = 0;
>> + clearSelect(db, pNewTmp, pNewTmp != &standin);
>> + pNewTmp = 0;
>> } else {
>> - assert(pNew->pSrc != 0 || pParse->is_aborted);
>> + assert(pNewTmp->pSrc != 0 || pParse->is_aborted);
>> }
>> - assert(pNew != &standin);
>> + assert(pNewTmp != &standin);
>> return pNew;
>> }
>>
>> --
>> 2.28.0
>>
next prev parent reply other threads:[~2020-11-25 9:52 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-20 2:23 [Tarantool-patches] [PATCH 0/2] GCC 10 warnings Artem Starshov
2020-11-20 2:23 ` [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10 Artem Starshov
[not found] ` <20201123231440.GA17397@tarantool.org>
2020-11-25 9:25 ` Artem
2020-11-25 9:52 ` Artem [this message]
2020-11-25 11:30 ` Nikita Pettik
2020-11-20 2:23 ` [Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build Artem Starshov
2020-11-21 11:29 ` Aleksandr Lyapunov
2020-11-25 11:29 ` Artem
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ee13a26-9079-9a3e-9bb9-68854fef6c43@tarantool.org \
--to=artemreyt@tarantool.org \
--cc=alexander.turenko@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox