[Maria-developers] MDEV-8372 Use helper methods introduced in MDEV-7824 all around the code
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. Thanks.
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.
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
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) 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. 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. 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
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.
Hi, Alexander! Ok to push, thanks! On Aug 10, Alexander Barkov wrote:
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));
ouch, that's kinda confusing. but ok, it's an old one, let's keep it for now.
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,
Only two places, I hoped there would be more... Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik