On Tue, Jul 14, 2020 at 2:02 PM vignesh C <vignes...@gmail.com> wrote: > On Tue, Jul 14, 2020 at 7:26 AM Amit Langote <amitlangot...@gmail.com> wrote: > > In CopyLoadRawBuf(), we could also change the condition if > > (cstate->raw_buf_index < cstate->raw_buf_len) to if (BUF_BYTES > 0), > > which looks clearer. > > > > Also, if we are going to use the macro more generally, let's make it > > look less localized. For example, rename it to RAW_BUF_BYTES similar > > to RAW_BUF_SIZE and place their definitions close by. It also seems > > like a good idea to make 'cstate' a parameter for clarity. > > > > Attached v6. > > > > Thanks for making the changes. > > - if (cstate->raw_buf_index < cstate->raw_buf_len) > + if (RAW_BUF_BYTES(cstate) > 0) > { > /* Copy down the unprocessed data */ > - nbytes = cstate->raw_buf_len - cstate->raw_buf_index; > + nbytes = RAW_BUF_BYTES(cstate); > memmove(cstate->raw_buf, cstate->raw_buf + > cstate->raw_buf_index, > nbytes); > } > > One small improvement could be to change it like below to reduce few > more instructions: > static bool > CopyLoadRawBuf(CopyState cstate) > { > int nbytes = RAW_BUF_BYTES(cstate); > int inbytes; > > /* Copy down the unprocessed data */ > if (nbytes > 0) > memmove(cstate->raw_buf, cstate->raw_buf + cstate->raw_buf_index, > nbytes); > > inbytes = CopyGetData(cstate, cstate->raw_buf + nbytes, > 1, RAW_BUF_SIZE - nbytes); > nbytes += inbytes; > cstate->raw_buf[nbytes] = '\0'; > cstate->raw_buf_index = 0; > cstate->raw_buf_len = nbytes; > return (inbytes > 0); > }
Sounds fine to me. Although CopyLoadRawBuf() does not seem to a candidate for rigorous code optimization as it does not get called that often. -- Amit Langote EnterpriseDB: http://www.enterprisedb.com