Skip to content

MDEV-39608: Derive command_lengths from command_array at compile time #5076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
keeper wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −14
Open

MDEV-39608: Derive command_lengths from command_array at compile time #5076

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions sql/sql_acl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
#include "sql_audit.h"
#include "password.h"
#include "scope.h"
#include <array>

#include "sql_plugin_compat.h"
#include "wsrep_mysqld.h"
Expand Down Expand Up @@ -9419,7 +9420,7 @@ static void add_user_parameters(THD *thd, String *result, ACL_USER* acl_user,
}
}

static const char *command_array[]=
static constexpr const char *command_array[]=
{
"SELECT", "INSERT", "UPDATE", "DELETE", "CREATE", "DROP", "RELOAD",
"SHUTDOWN", "PROCESS","FILE", "GRANT", "REFERENCES", "INDEX",
Expand All @@ -9432,23 +9433,18 @@ static const char *command_array[]=
"BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE"
};

static uint command_lengths[]=
{
6, 6, 6, 6, 6, 4, 6,
8, 7, 4, 5, 10, 5,
5, 14, 5, 23,
11, 7, 17, 14,
11, 9, 14, 13,
11, 5, 7, 17, 14,
8, 15, 16, 15,
23, 24, 12,
13, 13, 19
};
static constexpr auto command_lengths= []() constexpr {
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]));
Comment on lines +9437 to +9440
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Maintain consistency with the existing codebase style and types (uint vs uint32_t, array_elements vs std::size). (link)

Copy link
Copy Markdown
Member

@gkodinov gkodinov May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree with gemini here: please use the base type.

Copy link
Copy Markdown
Contributor Author

@keeper keeper May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return lengths;
}();


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,
Copy link
Copy Markdown
Member

@gkodinov gkodinov May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you are generating this there's no need for assert. Please remove it.

static_assert(command_lengths.size() == PRIVILEGE_T_MAX_BIT + 1,
"The definition of command_lengths does not match privilege_t");


Expand Down
Loading