MDEV-38144: update Optional_metadata_fields to use MariaDB types#4720
MDEV-38144: update Optional_metadata_fields to use MariaDB types#4720EricHayter wants to merge 1 commit intoMariaDB:mainfrom
Optional_metadata_fields to use MariaDB types#4720Conversation
|
Running |
Currently Optional_metadata_fields has many members that use classes from the C++ standard library, most notably the use of std::vector and std::string. These members have been updated to use existing MariaDB types instead.
I think it was determined that the coding standard wasn't acheivable with clang-format. I'm not sure if worth the effort. |
grooverdan
added
the
External Contribution
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Hi @EricHayter !
Thank you for submitting the PR! It is good to update our types to be consistent with MariaDB types. One thing this PR misses, that is in the description of MDEV-38144, is that rather than having an array per field, it would be better to have one array that has elements of a complex type which describe a field.
Previously, another contributor had prepared a patch, PR #4494, which looked to satisfy this; however, it was never thoroughly reviewed because they never got it to compile / pass tests on our CI system. I'd suggest looking through it for some inspiration (though I'm not sure of the quality of it).
Something the previous patch didn't try (though the JIRA mentions) is to use the Column_definition type defined in sql/field.h. That would be another improvement.
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
LGTM after reverting the space-only changes.
Please keep working with Brandon on the final review.
| @@ -42,7 +42,7 @@ template <typename Element_type> class Bounds_checked_array | |||
| {} | |||
|
|
|||
| void reset() { m_array= NULL; m_size= 0; } | |||
There was a problem hiding this comment.
please avoid space only changes.
| size_t old_size= elements(); | ||
| if (reserve(new_size)) | ||
| return true; | ||
There was a problem hiding this comment.
please avoid space only changes.
Noted. I'll take a look at that previous PR and update mine accordingly. Thanks. |
| @param[in] field SIGNEDNESS field in table_map_event. | ||
| @param[in] length length of the field | ||
| */ | ||
| static void parse_signedness(std::vector<bool> &vec, |
There was a problem hiding this comment.
Wait a minute.
The implementation of C++ std::vector<bool> may specialize to use a bit array rather than a resizable array of bools (which typically occupy the space of a char each).
Dynamic_array<bool> isn’t also specialized AFAIA.
Reference: https://www.en.cppreference.com/w/cpp/container/vector_bool.html
5 participants
Currently
Optional_metadata_fieldshas many members that use classes from the C++ standard library, most notably the use ofstd::vectorandstd::string. These members have been updated to use existing MariaDB types instead.