Clean up string_view usage in metadata_modifier#646
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
villagestevers
left a comment
villagestevers
left a comment
There was a problem hiding this comment.
Please look at the comment in helpers.cc FIRST. I did other files first and then realized the big problem with even doing this.
I think we should just look and see if some of these places can use string_view, but really the normalization functions cannot use string_views. Sorry for this being a little bit of a wild goose chase.
| if (matches.empty()) { | ||
| villagesql_error("Unknown %s '%s'", MYF(0), entity_type, name); | ||
| const std::string name_for_error(name); | ||
| villagesql_error("Unknown %s '%s'", MYF(0), entity_type, |
There was a problem hiding this comment.
You can use %.*s to use name directly with its length:
static_cast(name.length()), name.data() instead of name_for_error creation and .c_str()
| const std::string name_for_error(name); | ||
| villagesql_error("%s '%s' is ambiguous; qualify as %s", MYF(0), entity_type, | ||
| name, qualify_hint); | ||
| name_for_error.c_str(), qualify_hint); |
| " index type '%s' (extension '%s')", | ||
| MYF(0), col_type_name.c_str(), col_ext_name.c_str(), | ||
| index_type_name.c_str(), index_type_ext_name.c_str()); | ||
| index_type_name_for_error.c_str(), index_type_ext_name_for_error.c_str()); |
| : ""; | ||
| const std::string type_name(kci.custom_index_type.str, | ||
| kci.custom_index_type.length); | ||
| std::string_view ext_name = kci.custom_index_extension.str |
There was a problem hiding this comment.
Can't we just set this to to_string_view(kci.custom_index_extension)? it should be set to length 0 if not present.
| LEX_CSTRING prof = kp->get_index_profile(); | ||
| profile_name = std::string(prof.str, prof.length); | ||
| const std::string prof_extension = | ||
| std::string_view prof_name = to_string_view(prof); |
There was a problem hiding this comment.
Why introduce prof_name? if we have a string anyway, we can use that where string_view is being passed it somewhere.
| ? std::string(kp->get_index_profile_extension().str, | ||
| kp->get_index_profile_extension().length) | ||
| : ""; | ||
| ? to_string_view(kp->get_index_profile_extension()) |
There was a problem hiding this comment.
This should similarly be initialized properly. I don't like that it is initialized with {} but that should be equivalent to {nullptr, 0}. So it feels like the original call to has_index_profile_extension is probably extraneous.
| THD *thd, const std::string &extension_name, enum_mdl_duration duration) { | ||
| ExtensionKey ext_key(extension_name); | ||
| THD *thd, std::string_view extension_name, enum_mdl_duration duration) { | ||
| ExtensionKey ext_key{std::string(extension_name)}; |
There was a problem hiding this comment.
It feels like we could do better here (but not in the scope of this change). Since the Key is essentially short-lived, we could just use string_views internally (though it might be tricky if we need them to own their memory in some scenarios). Maybe add a TODO(villagesql): comment here like:
// TODO(villagesql): consider supporting key "views" that use string_views instead of owning their strings.
| } | ||
|
|
||
| std::string normalize_database_name(const std::string &name) { | ||
| static std::string casedn_string(const CHARSET_INFO *cs, std::string_view name) { |
There was a problem hiding this comment.
No, this is not what we want to do. I am realizing now that the downstream have to be strings. Notice this just wraps casedn and makes a string. The reason is that casedn needs to be able to modify the underlying string (possibly even resizing it - though that really shouldn't be happening for us, as the charset should already be in the utf8_mb4 family). This makes me now doubt that we even should do this change.
|
If it is easier to discuss over discord, you can join us there |
2 participants
Summary
Fixes #642.
This updates
metadata_modifierand related helper APIs to usestd::string_viewfor string inputs that are only read.Changes
std::string_viewmetadata_modifierhelper functions to avoid unnecessarystd::stringcopiesstd::string_viewstd::stringcopies where metadata is stored beyond the current callmetadata_modifierstring parameters tostd::string_viewValidation
git diff --checkpassedscripts/villint.shdue to local CRLF line-ending issueCloses #642