Re: bzr commit into mysql-azalea branch (tor.didriksen:3357)

[Konstantin Osipov]

* Tor Didriksen <Tor.Didriksen@stripped> [09/06/17 12:19]:
>  3357 Tor Didriksen	2009-06-16
>       http://bugs.mysql.com/bug.php?id=45523

Please add a bug title and describe the idea of the fix.

E.g. :

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() method that would transfer ownership of elements
from one list to another. 


Notice the style in which I wrote bug id above: Bug#XXX triggers
various automatic actions upon commit, so it's better to stick to
it.


> 
>     modified:
>       sql/sp_head.cc
>       sql/sql_cursor.cc
>       sql/sql_list.h
>       sql/sql_prepare.cc

Normally we write per-file comments as well. Your change is small,
so perhaps they are not necessary, but usually we're pretty
verbose.


> === 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-16 13:17:15 +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);

OK.

>    /*
>      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);

OK.

>    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-16 13:17:15 +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;
>  }

OK. 

> === 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-16 13:17:15 +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,20 @@ public:
>    {
>      return (first != &last) ? first : 0;
>    }
> -  friend class base_list_iterator;
> +
> +  /* Moves list elements to new owner. */

Please use doxygen changeset comments.

Why do you assert that new_owner is empty? It should be documented
in the method comment.

If your use cases are always such that new_owner is empty, perhaps
you should implement ::swap() instead, that would exchange two
lists? The advantage of swap(), in my view, is that it's a more
commonly used pattern. swap() can be used in all your use cases,
plus in a variety of others.

> +  void move_elements(base_ilist* new_owner) {

Opening bracket should belong to an own line.

> +    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& rhs);

& goes with rhs: const base_ilist &rhs, or just drop the parameter
name altogether.

>  };
>  
>  
> @@ -573,6 +587,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) {

  * goes with new_owner, 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-16 13:17:15 +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);

OK. If you use swap(), please restore that assert back.

>  
>    /* Items and memory will freed in destructor */

Have you run the regression tests?

--