maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Mon, Mar 29, 2021 at 3:55 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Michael!
>
> On Mar 29, Michael Widenius wrote:
> > revision-id: 1bbc852ef71 (mariadb-10.5.2-525-g1bbc852ef71)
> > parent(s): 039f4a054bb
> > author: Michael Widenius <michael.widenius@xxxxxxxxx>
> > committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> > timestamp: 2021-03-24 14:31:54 +0200
> > message:
> >
> > Changed field_index to use field_index_t instead of uint16
>
> Just curious. Why?

Mainly because it makes the code easier to read:

 find_field_in_table(THD *thd, TABLE *table, const char *name, size_t length,
-                    bool allow_rowid, uint16 *cached_field_index_ptr)
+                    bool allow_rowid, field_index_t *cached_field_index_ptr)
...
-static inline uint16 read_frm_fieldno(const uchar *data)
+static inline field_index_t read_frm_fieldno(const uchar *data)

With the new version, one can easier understand what the function expects
as arguments and what the function returns.

I had also seen different types used for field numbers in some
functions/classes and that also did bother me a bit so I decided to
fix this once and for all.

For example, in 10.5 we have:
item.h:  uint cached_field_index;

> > diff --git a/sql/unireg.cc b/sql/unireg.cc
> > index 51dd1fb6e8c..f1a0c5c5269 100644
> > --- a/sql/unireg.cc
> > +++ b/sql/unireg.cc
> > @@ -146,8 +146,8 @@ get_fieldno_by_name(HA_CREATE_INFO *create_info, List<Create_field> &create_fiel
> >    {
> >      if (field_name.streq(sql_field->field_name))
> >      {
> > -      DBUG_ASSERT(field_no <= uint16(~0U));
> > +      DBUG_ASSERT(field_no < NO_CACHED_FIELD_INDEX);
>
> this look suspicios. NO_CACHED_FIELD_INDEX is ((uint)(-1))
> which is always greater than anything you can put into field_index_t.

We have code that uses NO_CACHED_FIELD_INDEX in field_no
to indicate that there was no such field.

I looked at current code and we have:
#define NO_CACHED_FIELD_INDEX ((uint16) ~0)

> You want to ensure that field_no is valid when casted into
> field_index_t. I'd suggest to redefine NO_CACHED_FIELD_INDEX as
>
>     ((field_index_t)~0U)

I can redefine as that.

> > -      return uint16(field_no);
> > +      return (field_index_t) field_no;
>
> why not to declare field_no to be field_index_t? then you won't need a
> cast.

I have now fixed the above and and also changed type for the function
and also updated this in table.cc:
-static uint find_field(Field **fields, uchar *record, uint start, uint length);
+static field_index_t find_field(Field **fields, uchar *record, uint start,
+                                uint length);

I missed some of these cases as I did not get warnings from these from
the compiler...

Regards,
Monty