mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.75k stars 439 forks source link

Platform: replace use of std::string with String and StringView #559

Closed Squareys closed 2 years ago

Squareys commented 2 years ago

Hi @mosra!

Since all the std::string is removed elsewhere, it's now very worth removing it from GlfwApplication.h also!

Best, Jonathan

TODO

Squareys commented 2 years ago

@mosra Thanks for the review! This is more a "nice to have", since it will improve our compile times (eventually, when we're able to turn off MAGNUM_BUILD_DEPRECATED).

I might also have a look at making these changes for EmscriptenApplication tomorrow.

mosra commented 2 years ago

.. and SDL, and Android, and ... ;)

mosra commented 2 years ago

I took your changes as a base for a6da074ae0bb2198822e8f79f11c68d0a1bbbad1...53a74b5e6ce87920b2de5abcb8bde745c4b96524 and kept you as a co-author, but the changes are significantly different from what's in this PR. Tried to convince GitHub to show a diff between the two so you could see what additionally had to be done, but the best I got includes also a ton of other changes that happened since the PR was opened.

Things worth mentioning:

Squareys commented 2 years ago

I bet you use this quite a lot in your codebase, so maybe check which variant you have and make it consistent.

Quick note, because we're not using UTF8ToString() at all anymore: UTF8ToString() will call a JavaScript implementation of strlen in any case, it uses the second parameter only as a "buffer size, but the string might be shorter" parameter. We switched during the process of switching to StringView.

See also https://github.com/emscripten-core/emscripten/issues/12517.

mosra commented 2 years ago

it uses the second parameter only as a "buffer size, but the string might be shorter" parameter

Yes, I'm aware, but apart from the unnecessary strlen it's the behavior I want, to not make it any longer than the originating StringView is.

we're not using UTF8ToString() at all anymore

So you have your own implementation of this, based on https://github.com/emscripten-core/emscripten/issues/12517, and call that instead? In my case I'm not sure I'd want to bother (providing a JS file with a non-shitty implementation, bundling it to all produced executables, all that extra pain...).

The Stale Bot is a cancer, FFS. So many issues get just sweeped under the rug due to it.