Re: [Maria-developers] 1bbc852ef71: Changed field_index to use field_index_t instead of uint16
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@gmail.com> committer: Michael Widenius <michael.widenius@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?
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. 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)
- 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.
} }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Mon, Mar 29, 2021 at 3:55 PM Sergei Golubchik <serg@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.widenius@gmail.com> committer: Michael Widenius <michael.widenius@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
participants (2)
-
Michael Widenius -
Sergei Golubchik