Hi!
On Mon, Mar 29, 2021 at 3:55 PM Sergei Golubchik <s...@mariadb.org> 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.widen...@gmail.com>
> > committer: Michael Widenius <michael.widen...@gmail.com>
> > 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
_______________________________________________
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
