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 > _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
