Hi Sergei, On 06/02/2014 07:10 PM, Sergei Golubchik wrote:
Hi, Alexander!
Looks ok to push, thanks!
Thanks for review!
One minor comment below.
an answer to your comment below:
On Jun 02, Alexander Barkov wrote:
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-06-02 10:13:23 +0000 @@ -94,6 +94,31 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + return true; + case MYSQL_TYPE_DATETIME2: + case MYSQL_TYPE_TIMESTAMP2: + DBUG_ASSERT(0); // field->real_type() should not get to here.
In my earlier comment, where I suggested this assert, I've put it before other cases. That is, in optimized builds, when asserts are disabled, is_temporal_type_with_date() would still return true for MYSQL_TYPE_DATETIME2 and MYSQL_TYPE_TIMESTAMP2.
You've put it too late, it would return false for these types.
Admittedly, it's not terribly important, so fix it or not, as you like.
The MYSQL_TYPE_XXX2 types should not appear here. So returning false is Ok. And it can be faster, as in a release build the code is equal to: ... case MYSQL_TYPE_DATETIME2: case MYSQL_TYPE_TIMESTAMP2: default: return false; so a smart compiler should just remove any tests for MYSQL_TYPE_DATETIME2 and MYSQL_TYPE_TIMESTAMP2. (I didn't check though if gcc really removes the tests)
+ default: + return false; + } +}
Regards, Sergei