Skip to content
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.

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

string_view version of script::find_symbol_by_name and few others #30

Closed
Try opened this issue Nov 5, 2022 · 2 comments
Closed

string_view version of script::find_symbol_by_name and few others #30

Try opened this issue Nov 5, 2022 · 2 comments
Assignees
Milestone

Comments

@Try
Copy link
Contributor

Try commented Nov 5, 2022

Current syntax:
symbol* script::find_symbol_by_name(const std::string& name) forces client to bake std::string into interface.
for find function this is especially unnecessary, since it does string copy immediatly after:

	symbol* script::find_symbol_by_name(const std::string& name) {
		std::string up {name}; // <-- fine, but no need to query name as string
		std::transform(up.begin(), up.end(), up.begin(), ::toupper);

		if (auto it = _m_symbols_by_name.find(up); it != _m_symbols_by_name.end()) {
			return find_symbol_by_index(it->second);
		}

		return nullptr;
	}

In general it's probably would be good to pass read-only strings as string_view in all cases. If implementation then uses this string as key to has-map or similar - solve this locally inside the function.

Other places:

R vm::call_function(const std::string& name, P... args)
vm::init_instance(const std::string& name)
vm::allocate_instance(const std::string& name)
void vm::push_string(const std::string& value)
void register_external(const std::string& name, const std::function<R(P...)>& callback)
void override_function(const std::string& name, const std::function<R(P...)>& callback)
const way_point* way_net::waypoint(const std::string& name) const
const message_block* messages::block_by_name(const std::string& name) const
@lmichaelis
Copy link
Member

Partial duplicate of #21, but a good point. Until I can figure out a C++17 compatible way of using string-views all the way down, this is probably a good solution; especially for case-insensitive strings.

@lmichaelis lmichaelis added this to the v1.1.0 milestone Nov 6, 2022
@lmichaelis lmichaelis self-assigned this Nov 6, 2022
lmichaelis added a commit that referenced this issue Nov 6, 2022
Since we do a copy internally anyways, it doesn't hurt to allow users to
pass a `std::string_view` instead of a `std::string` which might avoid
unnecessary string copies. This change cascades to many other APIs related
to the script and VM.
lmichaelis added a commit that referenced this issue Nov 6, 2022
This was found due to 0f58174 in response
to #30 which proposes changing the `way_net::waypoint` API to take a
`std::string_view` instead of a `const std::string&`.
lmichaelis added a commit that referenced this issue Nov 6, 2022
This will reduce the number of small allocations needed for the underlying
`std::unordered_map`. It also allows `way_net::waypoint` to take a
`std::string_view` instead of a `const std::string&`
lmichaelis added a commit that referenced this issue Nov 8, 2022
This was found due to 0f58174 in response
to #30 which proposes changing the `way_net::waypoint` API to take a
`std::string_view` instead of a `const std::string&`.
@lmichaelis
Copy link
Member

Since I've been able to implement most of these as std::string_view I'm closing this issue. I have deprecated waynet::waypoint since that API is broken anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants