From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0B04B29335 for ; Thu, 7 Mar 2019 12:34:25 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UTUjLH9i1OR5 for ; Thu, 7 Mar 2019 12:34:24 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B526E28BEA for ; Thu, 7 Mar 2019 12:34:24 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 2/7] sql: rework sql_src_list_enlarge to set diag References: <73774bba8d68b70c0c6d5529ad30df4875370e87.1551265819.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9b814291-6165-b96f-8843-a0d67522ca40@tarantool.org> Date: Thu, 7 Mar 2019 20:34:22 +0300 MIME-Version: 1.0 In-Reply-To: <73774bba8d68b70c0c6d5529ad30df4875370e87.1551265819.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov In the commit header you said: "sql: rework sql_src_list_enlarge to set diag" But before your patch symbol 'sql_src_list_enlarge' does not exist. Please, fix that, and in other commits, who change function names, too. On 27/02/2019 14:13, Kirill Shcherbatov wrote: > Refactored sql_src_list_enlarge routine to use diag_set in case You've refactored sqlSrcListEnlarge, not sql_src_list_enlarge. > of memory allocation error. This will ensure that the > sqlSrcListAppend function throws an error using diag in > subsequent patches. > > Needed for #3931 See 4 comments below. > --- > src/box/sql/build.c | 109 ++++++++++++++++--------------------------- > src/box/sql/select.c | 28 ++++++----- > src/box/sql/sqlInt.h | 27 ++++++++++- > 3 files changed, 83 insertions(+), 81 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 3a65eb5a1..5337df450 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2597,76 +2597,45 @@ sqlIdListIndex(IdList * pList, const char *zName) > +struct SrcList * > +sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots, > + int start_idx) > +{ > + assert(start_idx >= 0); > + assert(new_slots >= 1); > + assert(src_list != NULL); > + assert(start_idx <= src_list->nSrc); > + > + /* Allocate additional space if needed. */ > + if (src_list->nSrc + new_slots > (int)src_list->nAlloc) { > + int to_alloc = src_list->nSrc * 2 + new_slots; > + int size = sizeof(*src_list) + > + (to_alloc - 1) * sizeof(src_list->a[0]); > + src_list = sqlDbRealloc(db, src_list, size); > + if (src_list == NULL) { > + diag_set(OutOfMemory, size, "sqlDbRealloc", "src_list"); > + return NULL; > } > - pSrc = pNew; > - nGot = > - (sqlDbMallocSize(db, pNew) - > - sizeof(*pSrc)) / sizeof(pSrc->a[0]) + 1; > - pSrc->nAlloc = nGot; > + src_list->nAlloc = > + (sqlDbMallocSize(db, src_list) - > + sizeof(*src_list)) / sizeof(src_list->a[0]) + 1;; 1. Double semicolon. 2. Not sure if it is worth using sqlDbMallocSize() here. Looks like double reserve. Firstly, we already increased size twice + new_slots (look at to_alloc). Secondly, we hope, that sqlDbRealloc will probably realloc more than necessary, but as I see, sqlDbRealloc will reserve at most +15 bytes. Please, just use src_list->nAlloc = to_alloc. > } > > - /* Move existing slots that come after the newly inserted slots > - * out of the way > + /* > + * Move existing slots that come after the newly inserted > + * slots out of the way. > */ > - for (i = pSrc->nSrc - 1; i >= iStart; i--) { > - pSrc->a[i + nExtra] = pSrc->a[i]; > - } > - pSrc->nSrc += nExtra; > + for (int i = src_list->nSrc - 1; i >= start_idx; i--) > + src_list->a[i + new_slots] = src_list->a[i]; 3. It is just memmove. Please use memmove() for that. > + src_list->nSrc += new_slots; > > - /* Zero the newly allocated slots */ > - memset(&pSrc->a[iStart], 0, sizeof(pSrc->a[0]) * nExtra); > - for (i = iStart; i < iStart + nExtra; i++) { > - pSrc->a[i].iCursor = -1; > - } > + /* Zero the newly allocated slots. */ > + memset(&src_list->a[start_idx], 0, sizeof(src_list->a[0]) * new_slots); > + for (int i = start_idx; i < start_idx + new_slots; i++) > + src_list->a[i].iCursor = -1; > > - /* Return a pointer to the enlarged SrcList */ > - return pSrc; > + /* Return a pointer to the enlarged SrcList. */ > + return src_list; > } > > struct SrcList * > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index ce29dc0b1..ea6476931 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3426,7 +3426,32 @@ void sqlInsert(Parse *, SrcList *, Select *, IdList *, > void *sqlArrayAllocate(sql *, void *, int, int *, int *); > IdList *sqlIdListAppend(sql *, IdList *, Token *); > int sqlIdListIndex(IdList *, const char *); > -SrcList *sqlSrcListEnlarge(sql *, SrcList *, int, int); > + > +/** > + * Expand the space allocated for the given SrcList object by > + * creating new_slots new slots beginning at start_idx. > + * The start_idx is zero based. New slots are zeroed. > + * > + * For example, suppose a SrcList initially contains two entries: > + * A,B. > + * To append 3 new entries onto the end, do this: > + * sql_src_list_enlarge(db, pSrclist, 3, 2); > + * > + * After the call above it would contain: A, B, nil, nil, nil. > + * If the iStart argument had been 1 instead of 2, then the result 4.1. You have no iStart. > + * would have been: A, nil, nil, nil, B. To prepend the new slots, > + * the iStart value would be 0. The result then would 4.2. The same. > + * be: nil, nil, nil, A, B. > + * @param db The database connection. > + * @param src_list The SrcList to be enlarged. > + * @param new_slots Number of new slots to add to src_list->a[]. > + * @param start_idx Index in pSrc->a[] of first new slot. 4.3. pSrc does not exist. > + * @retval not NULL SrcList pointer on success. > + * @retval NULL otherwise. 4.4. Please, start sentences with capital letters. > + */ > +struct SrcList * > +sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots, > + int start_idx); > > /** > * Allocate a new empty SrcList object. > -- > 2.20.1 > >