From: Tor Didriksen
Date: December 1 2010 2:44pm
Subject: Re: bzr commit into mysql-trunk branch (tor.didriksen:3247) WL#1393
List-Archive: http://lists.mysql.com/commits/125669
Message-Id: <4CF65F5D.2000501@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

On 2010-11-29 11:35, Øystein Grøvlen wrote:
> On 11/29/10 11:03 AM, Jorgen Loland wrote:
> ...
>
> > > === added file 'sql/bounded_queue.h'
> > > --- sql/bounded_queue.h 1970-01-01 00:00:00 +0000
> > > +++ sql/bounded_queue.h 2010-11-26 14:13:02 +0000
> > > @@ -0,0 +1,199 @@
> > > (...)
> > > + Key_type **pop()
> > > + {
> > > + // Don't return the extra element to the client code.
> > > + if (queue_is_full((&m_queue)))
> > > + queue_remove(&m_queue, 0);
> > > + return reinterpret_cast<Key_type**>(queue_remove(&m_queue, 0));
> > > + }
> >
> > JL: I suggest a DBUG_ASSERT(m_queue.elements>0); and a unit test that
> > hits it.
> >
>
> I do not think we should require that the caller needs to keep track
> of how many elements there are in the queue.  I think it is better if
> NULL is returned when the queue is empty.
>

Yes, the caller *should* keep track of how many elements there are in 
the queue.
Popping an empty queue is undefined behaviour.

> ...
>
> > > - param.keys--; /* TODO: check why we do this */
> > > param.sort_form= table;
> > > param.end=(param.local_sortorder=sortorder)+s_length;
> > > - if ((records=find_all_keys(&param,select,sort_keys, 
> &buffpek_pointers,
> > > - &tempfile, selected_records_file)) ==
> > > + if ((num_rows= find_all_keys(&param, select, sort_keys,
> > &buffpek_pointers,
> > > + &tempfile,
> > > + pq.is_initialized() ? &pq : NULL,
> > > + &found_rows)) ==
> >
> > JL: Again, I prefer assignments outside the if()
> >
> > num_rows= find_all_keys();
> > if (num_rows==HA_POS_ERROR){}
>
> Tor: I think this is a better improvement than my suggestion to assign
> 'pq.is_initialized() ? &pq : NULL' to a separate variable to make this
> more readable.  I following Jørgen suggestion, I think you can ignore
> my suggstion.

OK

>
> -- 
> Øystein
>
>
>
>