From: Martin Hansson
Date: December 20 2010 8:51am
Subject: Re: bzr commit into mysql-trunk-bugfixing branch (martin.hansson:3260)
 Bug#58918
List-Archive: http://lists.mysql.com/commits/127272
Message-Id: <4D0F18F5.2050901@oracle.com>
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="------------040504010002000505040206"

--------------040504010002000505040206
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Øystein Grøvlen skrev 2010-12-17 13.28:
> Hi Martin,
>
> Thanks for a good initiative.  I have bitten by this memcpy stuff 
> myself. I wonder whether I should require or just suggest that you try 
> to add a unit test for your clone methods.  Unit tests for basic 
> classes like Field should be possible without too much work.  (Tor 
> told me it was pretty straight-forward for Item_int).
It could of course be interesting to get into the unit testing 
framework, I haven't had time for that in the past. But I am not sure 
what I should be testing. Any suggestions? The only thing that needs to 
be tested IMHO is that every clone method returns an object of precisely 
the class for which is it's called and not a descendant class. But to 
test that requires RTTI - which we don't have - doesn't it?

Should I just instantiate every Field subclass and call clone on it?
>
> I also think that we should drop the existing clone method:
>
> >     Field *clone(MEM_ROOT *mem_root, TABLE *new_table);
>
> This is used in only one place, and its implementation uses only 
> public Field methods, so I think the entire code should be moved to 
> the caller.
> (The Field interface is unecessarily large. No need to provide 
> unecessary helper functions.)
Sure, I'll do that. I didn't look into this at all. Attached please find 
a patch for only this. Please scrutinize it.

Best Regards

Martin

--------------040504010002000505040206
Content-Type: text/plain; x-mac-type="0"; x-mac-creator="0";
 name="kill_clone.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="kill_clone.patch"

=== modified file 'sql/field.cc'
--- sql/field.cc	2010-12-16 13:49:35 +0000
+++ sql/field.cc	2010-12-20 08:38:33 +0000
@@ -1845,21 +1845,6 @@
 }
 
 
-/* This is used to generate a field in TABLE from TABLE_SHARE */
-
-Field *Field::clone(MEM_ROOT *root, TABLE *new_table)
-{
-  Field *tmp= clone(root);
-  if (tmp != NULL)
-  {
-    tmp->init(new_table);
-    tmp->move_field_offset((my_ptrdiff_t) (new_table->record[0] -
-                                           new_table->s->default_values));
-  }
-  return tmp;
-}
-
-
 /****************************************************************************
   Field_null, a field that always return NULL
 ****************************************************************************/

=== modified file 'sql/field.h'
--- sql/field.h	2010-12-16 13:49:35 +0000
+++ sql/field.h	2010-12-20 08:35:23 +0000
@@ -336,7 +336,6 @@
   virtual Field *new_key_field(MEM_ROOT *root, TABLE *new_table,
                                uchar *new_ptr, uchar *new_null_ptr,
                                uint new_null_bit);
-  Field *clone(MEM_ROOT *mem_root, TABLE *new_table);
 
   /**
      Makes a shallow copy of the Field object.

=== modified file 'sql/table.cc'
--- sql/table.cc	2010-11-23 22:37:59 +0000
+++ sql/table.cc	2010-12-20 08:42:30 +0000
@@ -1876,8 +1876,13 @@
   /* Setup copy of fields from share, but use the right alias and record */
   for (i=0 ; i < share->fields; i++, field_ptr++)
   {
-    if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam)))
+    Field *new_field= share->field[i]->clone(&outparam->mem_root);
+    *field_ptr= new_field;
+    if (new_field == NULL)
       goto err;
+    new_field->init(outparam);
+    new_field->move_field_offset((my_ptrdiff_t) (outparam->record[0] -
+                                                 outparam->s->default_values));
   }
   (*field_ptr)= 0;                              // End marker
 


--------------040504010002000505040206--