maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

  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