← Back to team overview

maria-developers team mailing list archive

Re: MDEV-16991 review.


  Hi Alexey,

On 11/25/2018 03:00 AM, Alexey Botchkov wrote:
> Hi, Alexander.
> First question about the sql/sql_basic_types.h.
> To me it looks overcomplicated. Do we really need classes over
> the simple enums?
> I attached my version of this file here. In my opinion it's shorter itself,
> and makes other code look nicer.

This is intentional, for strict data type control.
Simple enums have some problems. For example, you can pass
enums to a function that expects an integer data type.
I want any mistakes like this to cause a compilation error.

I would avoid using the traditional enums.
Can you please leave it as is?

I just found that C++11 introduced enum classes.
They don't convert to int implicitly, and don't
export their values to the public name space.


We have enabled C++11 in 10.4
So now this could be changed to:

class enum date_conv_mode_t
  CONV_NONE=          0U,
  FUZZY_DATES=        1U,
  TIME_ONLY=          4U,
  INTERVAL_hhmmssff=  8U,
  INTERVAL_DAY=      16U,
  NO_ZERO_DATE=      (1UL << 24),  // MODE_NO_ZERO_DATE

We could try this.

> Other than that 
> lines like this can be shortened
> -TIME_FUZZY_DATES            (date_mode_t::value_t::FUZZY_DATES),
> +TIME_FUZZY_DATES            (date_mode_t::FUZZY_DATES),

Thanks. Fixed.

> Three variables all defined differently. Looks funny :)
>     const date_conv_mode_t
>        ...
>     static const date_conv_mode_t
>     ...
>     static time_round_mode_t
> Why is that?

Thanks for noticing this. I've changed them all to "static const".

> What do you think if we do this:
>   - add THD* memver to the MYSQL_TIME_STATUS structure,
>     so we can get rid of the THD * parameter to many functions?
> - send one MYSQL_TIME_STATUS* to the add_nanoseconds()
>   instead of three {thd, &status->warnings, and status->nanoseconds} ?

Changing all functions that accept both &warn and nsec to
accept a pointer to MYSQL_TIME_STATUS instead, like this:

< xxxx(thd, &status->warnings, and status->nanoseconds);
> xxxx(thd, &status);

is a very good idea.

I'd avoid mix "thd" to MYSQL_TIME_STATUS though.
It's a pure C structure.

Does it sound OK?

> You do like this:
>    Datetime::Options opt(TIME_CONV_NONE, thd);
>    Datetime dt(thd, item->arguments()[0], opt);
> Why not just
>    Datetime dt(thd, item->arguments()[0], Datetime::Options
> opt(TIME_CONV_NONE, thd))?
> I'm not sure about this very case, but ofter code like yours translated
> with extra
> constructor call.

When the line fits the entire call, I do like this:

  Datetime dt(thd, item, Datetime::Options opt(mode, thd));

When the line does not fit, I tried both ways:

  Datetime dt(thd, item->arguments()[0],
             Datetime::Options opt(TIME_CONV_NONE, thd));


  Datetime::Options opt(TIME_CONV_NONE, thd);
  Datetime dt(thd, item->arguments()[0], opt);

and "c" looked easier to read for me than "b".

But this causes a constructor call indeed.
Although, in case of date_mode_t this is very cheap
(as cheap as copying of a 4 byte m_mode value), it's still
a good idea to avoid this. I'll change like you suggest.

Moreover, it's very likely that date_mode_t can change to something
mode complex in the future. So an extra constructor call can get more

> Best regards.
> HF

Follow ups