maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,



On 07/18/2015 12:18 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Jul 08, Alexander Barkov wrote:
> >     Hi Sergei,
> >
> > Please review my patch for mdev-8372.
> > I found only a few places to reuse the new methods.
> >
> > Also, had to add some more helper methods to make
> > the code clearer.

I removed all helper methods.

Though please see comments below why I initially added these methods.


> > Thanks.
>
> > diff --git a/sql/field.cc b/sql/field.cc
> > index 25506d6..bc040fa 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -10310,7 +10307,7 @@ bool Field::validate_value_in_record_with_warn(THD *thd, const uchar *record)
> >     {
> >       // Get and report val_str() for the DEFAULT value
> >       StringBuffer<MAX_FIELD_WIDTH> tmp;
> > -    val_str(&tmp, ptr_in_record(record));
> > +    val_str_in_record(&tmp, record);
>
> I don't think this makes the code any clearer


I added this for symmetry with is_null_in_record().

Removed now.


> >       push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
> >                           ER_INVALID_DEFAULT_VALUE_FOR_FIELD,
> >                           ER(ER_INVALID_DEFAULT_VALUE_FOR_FIELD),
> > diff --git a/sql/field.h b/sql/field.h
> > index da353f7..473927b 100644
> > --- a/sql/field.h
> > +++ b/sql/field.h
> > @@ -523,6 +523,10 @@ class Field
> >       my_ptrdiff_t l_offset= (my_ptrdiff_t) (record -  table->record[0]);
> >       return ptr + l_offset;
> >     }
> > +  const uchar *ptr_in_default_values() const
> > +  {
> > +    return ptr_in_record(table->s->default_values);
> > +  }
>
> And in particular, this certainly does not.
> Before your change I could see
>
>     ptr_in_record(table->s->default_values)

Without this method you can see:

 orig_field->ptr_in_record(orig_field->table->s->default_values)

Notice, two times "orig_field->".


And this looks particular ugly:

  String *res= orig_field->val_str(&tmp,
               orig_field->ptr_in_record(
               orig_field->table->s->default_values));

Notice, thee times "orig_field->".


I just don't like this kind of things, consider them buggy prone,
and prefer helper methods.



> in the code and I knew what it means if I knew what ptr_in_record()
> does. Now I additionally need to know what ptr_in_default_values() does
> or I need to jump to the method definition to see it.

Perhaps ptr_in_default_values_record() could be a more self-explanatory
name.


> I agree, it's a judgement call, but in my opinion the original
> expression is not long enough and isn't used often enough to justify a
> new method for it.

My concern was not about "long enough", but about multiple pointer 
references.

Anyway, removed all helper methods and rewrote the line with three
references to use a temporary variable.

Thanks.

> Same for other one-liners below.
>
> It's good to remove moving field ptr back and forth. But I don't think
> you need these wrappers for it.
>
> >     virtual void set_default()
> >     {
> >       my_ptrdiff_t l_offset= (my_ptrdiff_t) (table->s->default_values -
>
> Regards,
> Sergei



Btw, I'd propose some changes to distinguish between "const uchar *ptr"
and "const uchar *record":


1. Add a new simple class:

class Record
{
  const uchar *ptr;
};

2. Changed "default_values" from "uchar *" to "Record *",
and all other "uchar *" pointers that actually mean records.


There would be methods:

String *val_str(String *to, const uchar *ptr_arg);
String *val_str(String *to, const Record *record_arg);
String *val_str(String *to) { return val_str(to, ptr); }

Would be very logical, for my opinion.

Thanks.

diff --git a/sql/field.cc b/sql/field.cc
index 25506d6..e45cbdd 100644
--- a/sql/field.cc
+++ b/sql/field.cc
@@ -10077,16 +10077,14 @@ Create_field::Create_field(Field *old_field,Field *orig_field)
     if (!default_now)                           // Give a constant default
     {
       /* Get the value from default_values */
-      my_ptrdiff_t diff= orig_field->table->default_values_offset();
-      orig_field->move_field_offset(diff);	// Points now at default_values
-      if (!orig_field->is_real_null())
+      const uchar *dv= orig_field->table->s->default_values;
+      if (!orig_field->is_null_in_record(dv))
       {
         StringBuffer<MAX_FIELD_WIDTH> tmp(charset);
-        String *res= orig_field->val_str(&tmp);
+        String *res= orig_field->val_str(&tmp, orig_field->ptr_in_record(dv));
         char *pos= (char*) sql_strmake(res->ptr(), res->length());
         def= new Item_string(pos, res->length(), charset);
       }
-      orig_field->move_field_offset(-diff);	// Back to record[0]
     }
   }
 }
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index d59769b..e720db9 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -16477,20 +16477,17 @@ create_tmp_table(THD *thd, TMP_TABLE_PARAM *param, List<Item> &fields,
          inherit the default value that is defined for the field referred
          by the Item_field object from which 'field' has been created.
       */
-      my_ptrdiff_t diff;
-      Field *orig_field= default_field[i];
+      const Field *orig_field= default_field[i];
       /* Get the value from default_values */
-      diff= (my_ptrdiff_t) (orig_field->table->s->default_values-
-                            orig_field->table->record[0]);
-      orig_field->move_field_offset(diff);      // Points now at default_values
-      if (orig_field->is_real_null())
+      if (orig_field->is_null_in_record(orig_field->table->s->default_values))
         field->set_null();
       else
       {
         field->set_notnull();
-        memcpy(field->ptr, orig_field->ptr, field->pack_length());
+        memcpy(field->ptr,
+               orig_field->ptr_in_record(orig_field->table->s->default_values),
+               field->pack_length());
       }
-      orig_field->move_field_offset(-diff);     // Back to record[0]
     } 
 
     if (from_field[i])