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