brainboxdotcc / DPP

C++ Discord API Bot Library - D++ is Lightweight and scalable for small and huge bots!
https://dpp.dev/
Apache License 2.0
1.01k stars 155 forks source link

Use string views #1177

Closed BowDown097 closed 3 weeks ago

BowDown097 commented 3 weeks ago

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:

Code change checklist

netlify[bot] commented 3 weeks ago

Deploy Preview for dpp-dev ready!

Name Link
Latest commit 803ba5374ba212cef2e9036729e4ee0355b229f7
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...

QR Code

Use your smartphone camera to open QR code link.

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

braindigitalis commented 3 weeks ago

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.

BowDown097 commented 3 weeks ago

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