On Mon, 22 Jun 2009 13:57:17 +0200, Timour Katchaounov
<Timour.Katchaounov@stripped> wrote:
> Tor,
>
> Tor,
>
> I have just two comments/questions:
> - given your change, the comment below doesn't make sense:
> /*
> WARNING: copy constructor of this class does not create a usable
> copy, as its members may point at each other.
> */
>
> class base_ilist
>
> Instead you could comment above the copy constructor why it is
> forbidden.
Good point. done.
>
> - In several places the old code doesn't call the empty() method
> on the old list. Your patch makes this method called consistently.
> Did you check if some code actually relied on empty() not being
> called. I have not done this, but I have seen similar problems
> before with similar minor changes.
Yes, I did check this.
In some places the old code relied on reset_thd() to do the cleanup.
-- didrik
>
> Timour
>
>
>> #At file:///home/td136807/mysqldev/azalea-sql-list/ based on
>> revid:alik@stripped
>> 2802 Tor Didriksen 2009-06-18
>> Bug#45523 "Objects of class base_ilist should not be copyable".
>> Suppress the compiler-generated public copy constructor
>> and assignment operator of class base_ilist; instead, implement
>> move_elements() function which transfers ownership of elements
>> from one list to another.
>> Tested with:
>> BUILD/compile-pentium-debug-max
>> mysql-test/mtr --mem --force
>> @ sql/sp_head.cc
>> Use base_ilist::move_elements() rather than assignment operator
>> to move elements from thd->change_list to a new owner
>> @ sql/sql_cursor.cc
>> Use base_ilist::move_elements() rather than assignment operator
>> to move elements from thd->change_list to a new owner
>> @ sql/sql_list.h
>> make member variables first and last private
>> fix typo in 'friend class ...iterator'
>> disable the compiler generated copy CTOR and assignment operator
>> implement new function base_ilist::move_elements() instead
>> @ sql/sql_prepare.cc
>> Use base_ilist::move_elements() rather than assignment operator
>> to move elements from thd->change_list to a new owner
>> modified:
>> sql/sp_head.cc
>> sql/sql_cursor.cc
>> sql/sql_list.h
>> sql/sql_prepare.cc
>> === modified file 'sql/sp_head.cc'
>> --- a/sql/sp_head.cc 2009-06-12 02:01:08 +0000
>> +++ b/sql/sp_head.cc 2009-06-18 13:48:08 +0000
>> @@ -1174,8 +1174,7 @@ sp_head::execute(THD *thd)
>> We should also save Item tree change list to avoid rollback
>> something
>> too early in the calling query.
>> */
>> - old_change_list= thd->change_list;
>> - thd->change_list.empty();
>> + thd->change_list.move_elements(&old_change_list);
>> /*
>> Cursors will use thd->packet, so they may corrupt data which was
>> prepared
>> for sending by upper level. OTOH cursors in the same routine can
>> share this
>> @@ -1321,9 +1320,7 @@ sp_head::execute(THD *thd)
>> /* Restore all saved */
>> old_packet.swap(thd->packet);
>> DBUG_ASSERT(thd->change_list.is_empty());
>> - thd->change_list= old_change_list;
>> - /* To avoid wiping out thd->change_list on old_change_list
>> destruction */
>> - old_change_list.empty();
>> + old_change_list.move_elements(&thd->change_list);
>> thd->lex= old_lex;
>> thd->query_id= old_query_id;
>> DBUG_ASSERT(!thd->derived_tables);
>> === modified file 'sql/sql_cursor.cc'
>> --- a/sql/sql_cursor.cc 2009-01-26 16:03:39 +0000
>> +++ b/sql/sql_cursor.cc 2009-06-18 13:48:08 +0000
>> @@ -320,7 +320,7 @@ Sensitive_cursor::post_open(THD *thd)
>> lock= thd->lock;
>> query_id= thd->query_id;
>> free_list= thd->free_list;
>> - change_list= thd->change_list;
>> + thd->change_list.move_elements(&change_list);
>> reset_thd(thd);
>> /* Now we have an active cursor and can cause a deadlock */
>> thd->lock_info.n_cursors++;
>> @@ -436,7 +436,7 @@ Sensitive_cursor::fetch(ulong num_rows)
>> thd->open_tables= open_tables;
>> thd->lock= lock;
>> thd->query_id= query_id;
>> - thd->change_list= change_list;
>> + change_list.move_elements(&thd->change_list);
>> /* save references to memory allocated during fetch */
>> thd->set_n_backup_active_arena(this, &backup_arena);
>> @@ -458,7 +458,7 @@ Sensitive_cursor::fetch(ulong num_rows)
>> /* Grab free_list here to correctly free it in close */
>> thd->restore_active_arena(this, &backup_arena);
>> - change_list= thd->change_list;
>> + thd->change_list.move_elements(&change_list);
>> reset_thd(thd);
>> for (info= ht_info; info->read_view; info++)
>> @@ -505,7 +505,7 @@ Sensitive_cursor::close()
>> info->ht= 0;
>> }
>> - thd->change_list= change_list;
>> + change_list.move_elements(&thd->change_list);
>> {
>> /*
>> XXX: Another hack: we need to set THD state as if in a fetch to
>> be
>> @@ -531,7 +531,6 @@ Sensitive_cursor::close()
>> join= 0;
>> stmt_arena= 0;
>> free_items();
>> - change_list.empty();
>> DBUG_VOID_RETURN;
>> }
>> === modified file 'sql/sql_list.h'
>> --- a/sql/sql_list.h 2009-03-28 00:27:25 +0000
>> +++ b/sql/sql_list.h 2009-06-18 13:48:08 +0000
>> @@ -511,8 +511,9 @@ template <class T> class I_List_iterator
>> class base_ilist
>> {
>> + struct ilink *first;
>> + struct ilink last;
>> public:
>> - struct ilink *first,last;
>> inline void empty() { first= &last; last.prev= &first; }
>> base_ilist() { empty(); }
>> inline bool is_empty() { return first == &last; }
>> @@ -540,7 +541,27 @@ public:
>> {
>> return (first != &last) ? first : 0;
>> }
>> - friend class base_list_iterator;
>> +
>> + /**
>> + Moves list elements to new owner, and empties current owner (i.e.
>> this).
>> +
>> + @param[in,out] new_owner The new owner of the list elements.
>> + Should be empty in input.
>> + */
>> +
>> + void move_elements(base_ilist *new_owner)
>> + {
>> + DBUG_ASSERT(new_owner->is_empty());
>> + new_owner->first= first;
>> + new_owner->last= last;
>> + empty();
>> + }
>> +
>> + friend class base_ilist_iterator;
>> + private:
>> + /* Declare, but don't define copy CTOR and assignment operator. */
>> + base_ilist(const base_ilist&);
>> + void operator=(const base_ilist&);
>> };
>> @@ -573,6 +594,9 @@ public:
>> inline void push_back(T* a) { base_ilist::push_back(a); }
>> inline T* get() { return (T*) base_ilist::get(); }
>> inline T* head() { return (T*) base_ilist::head(); }
>> + inline void move_elements(I_List<T>* new_owner) {
>> + base_ilist::move_elements(new_owner);
>> + }
>> #ifndef _lint
>> friend class I_List_iterator<T>;
>> #endif
>> === modified file 'sql/sql_prepare.cc'
>> --- a/sql/sql_prepare.cc 2009-06-06 00:02:04 +0000
>> +++ b/sql/sql_prepare.cc 2009-06-18 13:48:08 +0000
>> @@ -3429,7 +3429,8 @@ Prepared_statement::execute_server_runna
>> Statement stmt_backup;
>> bool error;
>> Query_arena *save_stmt_arena= thd->stmt_arena;
>> - Item_change_list save_change_list= thd->change_list;
>> + Item_change_list save_change_list;
>> + thd->change_list.move_elements(&save_change_list);
>> state= CONVENTIONAL_EXECUTION;
>> @@ -3439,7 +3440,6 @@ Prepared_statement::execute_server_runna
>> thd->set_n_backup_statement(this, &stmt_backup);
>> thd->set_n_backup_active_arena(this, &stmt_backup);
>> thd->stmt_arena= this;
>> - thd->change_list.empty();
>> error= server_runnable->execute_server_code(thd);
>> @@ -3454,9 +3454,7 @@ Prepared_statement::execute_server_runna
>> thd->restore_backup_statement(this, &stmt_backup);
>> thd->stmt_arena= save_stmt_arena;
>> - DBUG_ASSERT(thd->change_list.is_empty());
>> - thd->change_list= save_change_list;
>> - save_change_list.empty();
>> + save_change_list.move_elements(&thd->change_list);
>> /* Items and memory will freed in destructor */
>>
>> ------------------------------------------------------------------------
>>
