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

Squareys · 2022-04-15T10:31:07Z

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

Best,
Jonathan

TODO

Compile the test
Run GlfwApplicationTest (currently crashes on some unset dpi policy?)

mosra

Err... it's not that simple 😅

And while at it, it'd be good to make corresponding changes in other application implementations, because otherwise e.g. switching between EmscriptenApplication and GlfwApplication may cause nasty behavioral differences.

... I guess I take over and finish this together with the other apps, eventually. Hopefully you don't need to rely on this too soon? :)

src/Magnum/Platform/GlfwApplication.cpp

mosra · 2022-04-15T15:57:21Z

src/Magnum/Platform/GlfwApplication.cpp

@@ -113,7 +113,7 @@ GlfwApplication::GlfwApplication(const Arguments& arguments, NoCreateT):
    else if(dpiScaling == "physical")
        _commandLineDpiScalingPolicy = Implementation::GlfwDpiScalingPolicy::Physical;
    #endif
-    else if(dpiScaling.find_first_of(" \t\n") != std::string::npos)
+    else if(dpiScaling.find(' ').data() || dpiScaling.find('\t').data() || dpiScaling.find('\n').data())


What's up with the .data()?

Actually, this change should wait until I expose an alternative to find_first_of(). It's there, just not public.

What's up with the .data()?

Ah, I guess you mean I could be using operator bool() instead? I see now, that the only empty view ever returned here would be the nullptr one 👍

Yeah that, but more like I should finish the API first so this doesn't need to be temporarily implemented via insufficient lacking APIs. Something like if(dpiScaling.containsAnyOf(...)) would be ideal.

src/Magnum/Platform/GlfwApplication.cpp

src/Magnum/Platform/GlfwApplication.h

Squareys · 2022-04-15T20:57:24Z

@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 · 2022-04-15T20:58:51Z

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

Signed-off-by: Squareys <squareys@googlemail.com>

mosra

Uh, I don't know but, like, the changes don't even compile 😅 I just gave up reviewing midway through because every change in the EmscriptenApplication was wrong one way or another.

What's the point of even pushing such a commit. Sorry to be so harsh, but it's easier for me to just discard the whole PR and start over, figuring out the changes from scratch, than attempting to review what's here and risk that I accidentally overlook a unhandled corner case in what looks like a hasty find&replace operation.

mosra · 2022-05-13T10:57:48Z

src/Magnum/Platform/EmscriptenApplication.cpp

-        std::string str = id;
-        std::free(id);
-        return str;
+        return Containers::String{id, std::strlen(id), &std::free};


The deleter takes a pointer and a size, so you can't just pass std::free() here.

mosra · 2022-05-13T10:58:09Z

src/Magnum/Platform/EmscriptenApplication.cpp

@@ -469,14 +467,14 @@ Vector2i EmscriptenApplication::framebufferSize() const {
 }
 #endif

-void EmscriptenApplication::setWindowTitle(const std::string& title) {
+void EmscriptenApplication::setWindowTitle(const Containers::StringView title) {
    #pragma GCC diagnostic push
    #pragma GCC diagnostic ignored "-Wdollar-in-identifier-extension"
    EM_ASM_({document.title = UTF8ToString($0);}, title.data());


mosra · 2022-05-13T10:58:22Z

src/Magnum/Platform/EmscriptenApplication.cpp

    #pragma GCC diagnostic push
    #pragma GCC diagnostic ignored "-Wdollar-in-identifier-extension"
    EM_ASM_({document.title = UTF8ToString($0);}, title.data());
    #pragma GCC diagnostic pop
 }

-void EmscriptenApplication::setContainerCssClass(const std::string& cssClass) {
+void EmscriptenApplication::setContainerCssClass(const Containers::StringView cssClass) {
    #pragma GCC diagnostic push
    #pragma GCC diagnostic ignored "-Wdollar-in-identifier-extension"
    EM_ASM_({


💥 here also

mosra · 2022-05-13T11:00:59Z

src/Magnum/Platform/EmscriptenApplication.cpp

@@ -605,7 +603,7 @@ void EmscriptenApplication::setupCallbacks(bool resizable) {
    }));
    #pragma GCC diagnostic pop

-    std::string keyboardListeningElementString;
+    Containers::StringView keyboardListeningElementString;


💥, again, and the += doesn't even compile.

mosra · 2022-05-26T14:18:29Z

I took your changes as a base for a6da074...53a74b5 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:

All functions that had to take care of non-null-terminated views are now tested in the corresponding application tests, by passing a slice out of a larger string. It's manual testing, but better than nothing.
The default application title now also uses a non-owning String, instead of always allocating a copy
Emscripten actually has an API to create a string from a pointer + size, so that's what I used instead of potentially allocating another copy. Important: the documentation of UTF8ToString() mentions that one should consistently use either the sized or the unsized variant, to make the JIT optimize the function better. I bet you use this quite a lot in your codebase, so maybe check which variant you have and make it consistent.
The TextInputEvent and TextEditingEvent used to use an ArrayView to avoid a string allocation. That was a temporary solution and they now use a StringView as well, which allowed me to remove an ArrayView.h include from all application headers, further reducing the bloat.

Squareys · 2022-05-30T07:02:12Z

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 emscripten-core/emscripten#12517.

mosra · 2022-05-30T09:17:40Z

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 emscripten-core/emscripten#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.

mosra requested changes Apr 15, 2022

View reviewed changes

Squareys force-pushed the glfw-application-remove-string branch from 8437538 to f7fc0e0 Compare April 15, 2022 20:55

Squareys added 2 commits April 18, 2022 21:25

Platform: replace std::string with String/StringView in GlfwApplication

0765268

Signed-off-by: Squareys <squareys@googlemail.com>

Platform: replace std::string with String/StringView in EmscriptenApp*

4768ea6

Signed-off-by: Squareys <squareys@googlemail.com>

Squareys force-pushed the glfw-application-remove-string branch from f7fc0e0 to 4768ea6 Compare April 18, 2022 19:27

Platform: replace std::string with String/StringView in Sdl2Application

953a684

Signed-off-by: Squareys <squareys@googlemail.com>

mosra reviewed May 13, 2022

View reviewed changes

mosra added the changelog mention added label May 25, 2022

mosra closed this May 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Squareys commented Apr 15, 2022 •

edited

Loading

mosra left a comment

mosra Apr 15, 2022

Squareys Apr 15, 2022

mosra Apr 15, 2022

Squareys commented Apr 15, 2022

mosra commented Apr 15, 2022

mosra left a comment

mosra May 13, 2022

mosra May 13, 2022

mosra May 13, 2022

mosra May 13, 2022

mosra commented May 26, 2022

Squareys commented May 30, 2022 •

edited

Loading

mosra commented May 30, 2022

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

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

Conversation

Squareys commented Apr 15, 2022 • edited Loading

mosra left a comment

Choose a reason for hiding this comment

mosra Apr 15, 2022

Choose a reason for hiding this comment

Squareys Apr 15, 2022

Choose a reason for hiding this comment

mosra Apr 15, 2022

Choose a reason for hiding this comment

Squareys commented Apr 15, 2022

mosra commented Apr 15, 2022

mosra left a comment

Choose a reason for hiding this comment

mosra May 13, 2022

Choose a reason for hiding this comment

mosra May 13, 2022

Choose a reason for hiding this comment

mosra May 13, 2022

Choose a reason for hiding this comment

mosra May 13, 2022

Choose a reason for hiding this comment

mosra commented May 26, 2022

Squareys commented May 30, 2022 • edited Loading

mosra commented May 30, 2022

Squareys commented Apr 15, 2022 •

edited

Loading

Squareys commented May 30, 2022 •

edited

Loading