MDEV-39608: Derive command_lengths from command_array at compile time#5076
MDEV-39608: Derive command_lengths from command_array at compile time#5076keeper wants to merge 1 commit into
Conversation
Replace the manually-maintained command_lengths array with a constexpr lambda that computes string lengths from command_array using std::char_traits<char>::length(). This eliminates the error-prone need to keep the two arrays in sync manually. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the command_lengths array in sql/sql_acl.cc to be computed at compile-time using a constexpr lambda and std::array, while also marking command_array as constexpr. The review feedback recommends maintaining consistency with the existing codebase by using the uint type and the array_elements macro instead of uint32_t and std::size.
| std::array<uint32_t, std::size(command_array)> lengths{}; | ||
| for (size_t i= 0; i < lengths.size(); i++) | ||
| lengths[i]= static_cast<uint32_t>( | ||
| std::char_traits<char>::length(command_array[i])); |
There was a problem hiding this comment.
The original code used the uint type for the elements of command_lengths. Using uint32_t here introduces an inconsistency with the rest of the file and the command_array logic. It is better to use uint to maintain consistency and avoid potential type mismatch warnings in other parts of the code that consume this array. Additionally, using array_elements(command_array) instead of std::size(command_array) is more consistent with the existing codebase style and the static_assert on line 9445.
std::array<uint, array_elements(command_array)> lengths{};
for (size_t i= 0; i < lengths.size(); i++)
lengths[i]= static_cast<uint>(
std::char_traits<char>::length(command_array[i]));References
- Maintain consistency with the existing codebase style and types (uint vs uint32_t, array_elements vs std::size). (link)
There was a problem hiding this comment.
I'd agree with gemini here: please use the base type.
There was a problem hiding this comment.
I intentionally switch to uint32_t because this jira:
https://jira.mariadb.org/browse/MDEV-35460
For std::size(), AFAIK MariaDB is moving toward C++17 and array_elements() is not type-safe, that's why I choose modern std::size(). So I'm curious about the preferred approach here: should style modernization (like uint32_t, std::size()) happen opportunistically in related refactors, or strictly in dedicated PRs?
I understand it's a different topic so I'm ok with reverting — just wondering what the approach is for phasing in C++17 idioms.
gkodinov
added
the
External Contribution
gkodinov
changed the title
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.
I've taken the liberty of filing a jira for you for this. But please consider doing that yourself going forward.
| std::array<uint32_t, std::size(command_array)> lengths{}; | ||
| for (size_t i= 0; i < lengths.size(); i++) | ||
| lengths[i]= static_cast<uint32_t>( | ||
| std::char_traits<char>::length(command_array[i])); |
There was a problem hiding this comment.
I'd agree with gemini here: please use the base type.
|
|
||
| static_assert(array_elements(command_array) == PRIVILEGE_T_MAX_BIT + 1, | ||
| "The definition of command_array does not match privilege_t"); | ||
| static_assert(array_elements(command_lengths) == PRIVILEGE_T_MAX_BIT + 1, |
There was a problem hiding this comment.
Now that you are generating this there's no need for assert. Please remove it.
|
Idiomatic MariaDB code would be to do LEX_CSTRING command_array[] = {
{ STRING_WITH_LEN("SELECT") },
... |
3 participants
Description
Replace the manually-maintained command_lengths array with a constexpr
lambda that computes string lengths from command_array using
std::char_traits::length(). This eliminates the error-prone need
to keep the two arrays in sync manually.
How can this PR be tested?
There's no functional change. The code should compile and pass exists MTR tests.
Copyright
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.