[Tarantool-patches] [PATCH v4 33/53] sql: introduce mem_append_to_binary()

[Mergen Imeev]

Thank you for the review! My answers below.


On Tue, Mar 30, 2021 at 01:05:44AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index 86da1449c..2e8669138 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1305,19 +1305,26 @@ sqlVdbeList(Vdbe * p)
> >  			 * has not already been seen.
> >  			 */
> >  			if (pOp->p4type == P4_SUBPROGRAM) {
> > -				int nByte = (nSub + 1) * sizeof(SubProgram *);
> >  				int j;
> >  				for (j = 0; j < nSub; j++) {
> >  					if (apSub[j] == pOp->p4.pProgram)
> >  						break;
> >  				}
> > -				if (j == nSub &&
> > -				    sqlVdbeMemGrow(pSub, nByte,
> > -						   nSub != 0) == 0) {
> > -					apSub = (SubProgram **) pSub->z;
> > -					apSub[nSub++] = pOp->p4.pProgram;
> > -					pSub->flags |= MEM_Blob;
> > -					pSub->n = nSub * sizeof(SubProgram *);
> > +				if (j == nSub) {
> > +					uint32_t size = sizeof(SubProgram *);
> > +					char *value = (char *)&pOp->p4.pProgram;
> > +					if (nSub == 0) {
> > +						if (mem_copy_binary(pSub, value,
> > +								    size) != 0)
> > +							return -1;
> > +					} else {
> > +						assert(0);
> 
> What is this assert? And why does not the append work on empty
> binaries?
> 
Thank you. This assert did't appear in tests because there is no two or more
triggers executed one after another in tests. I used it for debugging and forgot
to remove it. Fixed. Also, I reworked this part - instead of adding of a new
function (mem_append_to_binary()) I used mem_concat() here. It can be seen in
patch "sql: introduce mem_copy_bin()".

> > +						if (mem_append_to_binary(pSub,
> > +									 value,
> > +									 size) != 0)
> > +							return -1;
> > +					}
> > +					++nSub;
> >  				}
> >  			}
> >  		}
> >