Re: bzr commit into mysql-trunk branch (tor.didriksen:3247) WL#1393

[Øystein Grøvlen]

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.

...

 >  > - 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.

--
Øystein