Stack overflow in Label::create

[juj]

Running on the modernize branch very fresh off the oven, looks like this has crept in when entering the Options menu from the main menu:

The following change avoids the overflow, though not sure the intent with the std::string_view would be to avoid the std::string construction in the first place?

diff --git a/include/GUI/Label.h b/include/GUI/Label.h
index b65766aa..ca9afc1e 100644
--- a/include/GUI/Label.h
+++ b/include/GUI/Label.h
@@ -164,7 +164,7 @@ public:
     static std::unique_ptr<Label>
     create(std::string_view text, Uint32 textcolor = COLOR_DEFAULT, Uint32 textshadowcolor = COLOR_DEFAULT,
            Uint32 backgroundcolor = COLOR_TRANSPARENT) {
-        return create(text, textcolor, textshadowcolor, backgroundcolor);
+        return create(std::string(text), textcolor, textshadowcolor, backgroundcolor);
     }

 protected:

[henricj]

This is fallout from when I changed the translation code to pass std::string_view instead of const std::string &. https://github.com/henricj/dunelegacy/blob/531fc8acfb88161b58916e01b14415abc8dc232c/include/globals.h#L31 https://github.com/henricj/dunelegacy/blob/531fc8acfb88161b58916e01b14415abc8dc232c/include/FileClasses/TextManager.h#L77-L85

In code like where this bug manifested, https://github.com/henricj/dunelegacy/blob/531fc8acfb88161b58916e01b14415abc8dc232c/src/Menu/OptionsMenu.cpp#L66 it shouldn't make a whole lot of difference since the same const std::string & was passed all the way to Label::setText() (through the create() call and then the ctor), where a std::string was instantiated. But in many places the item returned from _() is used, directly or indirectly, to build larger strings or as an argument to libfmt, where the extra work to create the std::string from the string literal to pass as an argument is entirely wasted (also making it more difficult for the compiler to optimize since std::string's ctor is neither constexpr nor noexcept—well, there is a constepr ctor, but it can only be used where the resulting object does not escape a constexpr function since it sometimes does heap allocation).

From this, https://godbolt.org/z/GG9aY17aq

To this: https://godbolt.org/z/8hf6Wvjs9

Clang is better, turning the entire std::string_view example into this:

f():                                  # @f()
        mov     edi, 13
        mov     esi, offset .L.str
        jmp     getLocalized(std::basic_string_view<char, std::char_traits<char> >) # TAILCALL
.L.str:
        .asciz  "Hello, world!"

That said, I didn't expect changing getLocalized()'s signature to ripple out so widely. (Echoes of Knuth...)

[juj]

Thanks! Confirmed this to be fixed in the tree.