maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

  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.

https://www.codeguru.com/cpp/cpp/article.php/c19083/C-2011-Stronglytyped-Enums.htm

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,
  RANGE0_LAST=       INTERVAL_DAY,
  NO_ZERO_IN_DATE=   (1UL << 23),  // MODE_NO_ZERO_IN_DATE
  NO_ZERO_DATE=      (1UL << 24),  // MODE_NO_ZERO_DATE
  INVALID_DATES=     (1UL << 25)   // MODE_INVALID_DATES
};

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:

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


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

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

and

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


> 
> Best regards.
> HF
>