Use string views #1177

BowDown097 · 2024-06-21T08:30:37Z

This will make the library easier to use for std::string_view users as a LOT of explicit std::string creations are needed atm. There should be some performance improvements as well (less allocations, blablabla). Tested with my own bot which is pretty complex and works 100%.

This is completely backwards compatible in my testing, with the exception of implicit conversions. For example, within the library itself, there were some assignments of a utility::icon to a string that would implicitly create a utility::iconhash - now it does not, and does not compile without explicitly creating the iconhash. With my own bot this happened with std::smatch.

There's also some new stuff / changes outside of just changing std::strings to std::string_views:

A new utility function for maps, dpp::utility::emplace_or_assign, which essentially is insert_or_assign but with the in-place construction that emplace offers.
All non-floating point instances of std::stoX that would take in a std::string_view have been changed to use std::from_chars.
2 new types, dpp::membuf and dpp::imemstream, in stringops.h, which allow input streams over a raw character array, necessary for std::string_view. These are not compiled if std::ispanstream is available, which was added in C++23, as it is far superior. All instances of std::istringstream that would take in a std::string_view have been replaced to use one or the other, depending on what's available. (P.S. should these be exported with DPP_EXPORT?)

Code change checklist

I have ensured that all methods and functions are fully documented using doxygen style comments.
My code follows the coding style guide.
I tested that my change works before raising the PR.
I have ensured that I did not break any existing API calls.
I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

This will make the library easier to use for std::string_view users as a LOT of explicit std::string creations are needed atm. There should be some performance improvements as well (less allocations, blablabla). This is mostly backwards compatible, but will cause issues with implicit conversions. For example, within the library itself, there were some assignments of a utility::icon to a string that would implicitly create a utility::iconhash - now it does not, and does not compile without explicitly creating the iconhash. This could be accounted for with a bit more code, or just reverting the usage of std::string_view, if it's important enough. There's also some new stuff / changes outside of just changing std::strings to std::string_views: - A new utility function for maps, dpp::utility::emplace_or_assign, which essentially is insert_or_assign but with the in-place construction that emplace offers. - All non-floating point instances of std::stoX that would take in a std::string_view have been changed to use std::from_chars. - 2 new types, dpp::membuf and dpp::imemstream, in stringops.h, which allow input streams over a raw character array, necessary for std::string_view. These are not compiled if std::ispanstream is available, which was added in C++23, as it is far superior. All instances of std::istringstream that would take in a std::string_view have been replaced to use one or the other, depending on what's available.

netlify · 2024-06-21T08:31:05Z

✅ Deploy Preview for dpp-dev ready!

Name	Link
🔨 Latest commit	`803ba53`
🔍 Latest deploy log	https://app.netlify.com/sites/dpp-dev/deploys/66753a30bc972b0008214fc0
😎 Deploy Preview	https://deploy-preview-1177--dpp-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Jaskowicz1

Thanks for the contribution!

Please correct your indentation to comply with the Coding Style Standards. You're using spaces, not tabs.

Jaskowicz1 · 2024-06-21T09:14:18Z

include/dpp/stringops.h

@@ -78,10 +137,11 @@ inline std::string rtrim(std::string s)
 * @param s string to trim
 * @return std::string trimmed string
 */
-inline std::string ltrim(std::string s)
+inline std::string ltrim(std::string_view s)


Ditto the comment from rtrim.

Jaskowicz1 · 2024-06-21T09:15:30Z

include/dpp/stringops.h

 /**
 * @brief trim from end of string (right)
 * 
 * @param s String to trim
 * @return std::string trimmed string
 */
-inline std::string rtrim(std::string s)
+inline std::string rtrim(std::string_view s)


I don't agree with this change. You're now having to create a new string which adds more bytes (as you've now got the bytes from s + the bytes from s_cpy), which now adds 16 more bytes every time you call this function. This should be left as std::string.

braindigitalis · 2024-06-21T11:06:13Z

I'm not sure why every instance of a string parameter, plus various strings in objects must all be changed to string views. this is a huge change which is impossible to review and impossible to say what side effects it has as it changes literally every function.
how can we say this doesn't break things, the unit tests or one test bot don't scratch the surface for such a far reaching change.

it's better to concentrate on one subsystem at a time doing a gradual refactor than what looks like global search and replace.

string view is not a magic bullet.

braindigitalis

far too big to review sorry

Mishura4

So overall yes there are places that could use string_view, but blanket replacing all of them has no benefit and is even a downgrade. If someone passes a string which you convert to sv and then construct again a string, what's the benefit exactly?

We also don't need the c++23-specific features, keep in mind this is code we have to use and improve and, and making a change to something that uses c++23 if it's enabled and doesn't if not means we have to change and test both, which isn't tenable.

There are some positive changes in there but those could be in separate PRs that would be easier to review and test.

Also, be mindful of intendation changes.

Mishura4 · 2024-06-21T13:31:30Z

src/dpp/bignum.cpp

 	if (dpp::lowercase(number_string.substr(0, 2)) == "0x") {
-		BN_hex2bn(&ssl_bn->bn, number_string.substr(2, number_string.length() - 2).c_str());
+        BN_hex2bn(&ssl_bn->bn, std::string(number_string.substr(2)).c_str());


If you construct a std::string anyways then what's the point? You now have an extra allocation when someone passes an std::string

Mishura4 · 2024-06-21T13:32:24Z

src/dpp/cluster.cpp

-void cluster::post_rest(const std::string &endpoint, const std::string &major_parameters, const std::string &parameters, http_method method, const std::string &postdata, json_encode_t callback, const std::string &filename, const std::string &filecontent, const std::string &filemimetype, const std::string &protocol) {
-	rest->post_request(std::make_unique<http_request>(endpoint + (!major_parameters.empty() ? "/" : "") + major_parameters, parameters, [endpoint, callback](http_request_completion_t rv) {
+void cluster::post_rest(std::string_view endpoint, std::string_view major_parameters, std::string_view parameters, http_method method, std::string_view postdata, json_encode_t callback, std::string_view filename, std::string_view filecontent, std::string_view filemimetype, std::string_view protocol) {
+    rest->post_request(std::make_unique<http_request>(std::string(endpoint) + (!major_parameters.empty() ? "/" : "") + std::string(major_parameters), parameters, [callback](http_request_completion_t rv) {


Same thing, what's the point here of changing the parameter

Mishura4 · 2024-06-21T13:33:12Z

src/dpp/cluster.cpp

@@ -349,7 +349,7 @@ void cluster::post_rest_multipart(const std::string &endpoint, const std::string
 		file_mimetypes.push_back(data.mimetype);
 	}

-	rest->post_request(std::make_unique<http_request>(endpoint + (!major_parameters.empty() ? "/" : "") + major_parameters, parameters, [endpoint, callback](http_request_completion_t rv) {
+    rest->post_request(std::make_unique<http_request>(std::string(endpoint) + (!major_parameters.empty() ? "/" : "") + std::string(major_parameters), parameters, [callback](http_request_completion_t rv) {


Again, what's the point

Mishura4 · 2024-06-21T13:34:18Z

src/dpp/cluster/entitlement.cpp

@@ -62,15 +62,15 @@ void cluster::entitlements_get(snowflake user_id, const std::vector<snowflake>&

 	j["exclude_ended"] = exclude_ended;

-	rest_request_list<entitlement>(this, API_PATH "/applications", me.id.str(), "entitlements", m_get, j, callback);
+    rest_request_list<entitlement>(this, API_PATH "/applications", me.id.str(), "entitlements", m_get, std::string(j), callback);


Again what's the point™

I'll stop commenting that because there are tens of these

Mishura4 · 2024-06-21T13:35:30Z

src/dpp/commandhandler.cpp

 {
 	command_info_t i;
 	i.func = std::move(handler);
 	i.guild_id = guild_id;
 	i.parameters = parameters;
-	commands[lowercase(command)] = i;
+
+    std::string command_lower = lowercase(command);


This is good, could have been a separate PR

Mishura4 · 2024-06-21T13:39:41Z

src/dpp/dispatcher.cpp

-	reassign(vc, _user_id, pcm, length);
-}
-
-voice_receive_t::voice_receive_t(discord_client* client, std::string&& raw, discord_voice_client* vc, snowflake _user_id, const uint8_t* pcm, size_t length) : event_dispatch_t(client, std::move(raw)), voice_client(vc), user_id(_user_id) {


Why? This was good for performance, what it needed was add std::move to the usage of raw. With your change you have an extra allocation

Mishura4 · 2024-06-21T13:40:25Z

src/dpp/dns.cpp

 	{
 		addrinfo hints, *addrs;
 		dns_cache_t::const_iterator iter;
 		time_t now = time(nullptr);
 		int error;
 		bool exists = false;
+        std::string hostname_str(hostname);


Mishura4 · 2024-06-21T13:42:24Z

src/dpp/httpsclient.cpp

-const std::string https_client::get_header(std::string header_name) const {
-	std::transform(header_name.begin(), header_name.end(), header_name.begin(), [](unsigned char c){
+const std::string https_client::get_header(std::string_view header_name) const {
+    std::string header_name_str(header_name);


Extra alloc here again

Mishura4 · 2024-06-21T13:42:53Z

src/dpp/httpsclient.cpp

+        hci.hostname = url.substr(0, colon_pos);
+
+        std::string_view port_str = url.substr(colon_pos + 1, url.length());
+        auto [_, err] = std::from_chars(port_str.data(), port_str.data() + port_str.size(), hci.port);


This is good

Mishura4 · 2024-06-21T13:44:48Z

include/dpp/stringops.h

-	std::istringstream iss(s);
-	iss >> f, iss >> t;
+
+#ifdef __cpp_lib_spanstream


This will be harder to maintain for very little benefit

BowDown097 · 2024-06-21T14:50:05Z

Yeah, good points. I had an initial idea for this which is probably better and that I'll do instead.

github-actions bot added documentation Improvements or additions to documentation code Improvements or additions to code. labels Jun 21, 2024

Jaskowicz1 requested changes Jun 21, 2024

View reviewed changes

braindigitalis requested changes Jun 21, 2024

View reviewed changes

Mishura4 requested changes Jun 21, 2024

View reviewed changes

BowDown097 closed this Jun 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use string views #1177

Use string views #1177

BowDown097 commented Jun 21, 2024

netlify bot commented Jun 21, 2024 •

edited

Loading

Jaskowicz1 left a comment

Jaskowicz1 Jun 21, 2024

Jaskowicz1 Jun 21, 2024

braindigitalis commented Jun 21, 2024

braindigitalis left a comment

Mishura4 left a comment •

edited

Loading

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

Mishura4 Jun 21, 2024

BowDown097 commented Jun 21, 2024

Use string views #1177

Use string views #1177

Conversation

BowDown097 commented Jun 21, 2024

Code change checklist

netlify bot commented Jun 21, 2024 • edited Loading

✅ Deploy Preview for dpp-dev ready!

Jaskowicz1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braindigitalis commented Jun 21, 2024

braindigitalis left a comment

Choose a reason for hiding this comment

Mishura4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BowDown097 commented Jun 21, 2024

netlify bot commented Jun 21, 2024 •

edited

Loading

Mishura4 left a comment •

edited

Loading