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
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