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