maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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