MyGUI / mygui

Fast, flexible and simple GUI.
http://mygui.info/
Other
713 stars 205 forks source link

Switch to C++17 and use string_view #252

Closed Assumeru closed 1 year ago

Assumeru commented 1 year ago

As mentioned in #251.

This PR attempts to reduce the number of superfluous string allocations in MyGUI. Mostly by employing string_view almost everywhere, but also by fixing a few places that copied a (return) value instead of taking it by reference, and (in places I spotted the issue) by preventing redundant UString->string->UString conversion chains.

Some aspects of the API where left as const std::string&, mostly the ones that ended up calling Ogre functions taking arguments of that type. It might be worth changing more of those over to string_view in the future though such a change would necessarily favour the other rendering targets over Ogre in terms of performance -- minor though the difference might be.

This is a pretty big change with regard to backwards compatibility. Upgrading shouldn't be particularly hard, it wasn't for OpenMW.

I've tested compilation on Windows 10 / MSVC and Ubuntu 22 / Clang 14.

Assumeru commented 1 year ago

@psi29a maybe you can verify this compiles on Mac?

Altren commented 1 year ago

I can check that

psi29a commented 1 year ago

We an also setup github actions to do that for us. :) We can liberate what we've done over at openmw.

Altren commented 1 year ago

That would be great, since travis-ci is broken now

Altren commented 1 year ago

@Assumeru there are few review comments above before we can merge this

Altren commented 1 year ago

And thank you for you work, glad to see so big and good PR

Assumeru commented 1 year ago

@Assumeru there are few review comments above before we can merge this

The thing about cmake policies AnyOldName3 mentioned or the commits you added?

And thank you for you work, glad to see so big and good PR

Thanks for even being willing to entertain the notion of a 2k line PR 😅 Once this is merged I'll probably end up submitting some more to get rid of a few compilation warnings (implicit casts, c-style casts) and maybe to employ unique_ptr so we can eliminate a few destructors.

Altren commented 1 year ago

No, there are my review comments above. The only critical one is about eventClipboardChanged

Altren commented 1 year ago

Once this is merged I'll probably end up submitting some more to get rid of a few compilation warnings (implicit casts, c-style casts)

I'm waiting for this PR before pushing my massive changes, so don't rush into doing that. Also I somewhat prefer c-style casts for basic types, since otherwise code becomes less clean. So don't change that unless really necessary

Assumeru commented 1 year ago

No, there are my review comments above. The only critical one is about eventClipboardChanged

I'm not seeing any other comments. Did you leave them as a draft?

Altren commented 1 year ago

No, there are my review comments above. The only critical one is about eventClipboardChanged

I'm not seeing any other comments. Did you leave them as a draft?

@Assumeru my bad. sorry

AnyOldName3 commented 1 year ago

I don't have the button to unresolve https://github.com/MyGUI/mygui/pull/252#discussion_r1199626434 so I'm making another top-level comment so my last post has some visibility. I don't think the thread should have been resolved.

Altren commented 1 year ago

I now hope your planned changes involve removing the const_casts from MyGUI_Delegate.h somehow 😛

@Assumeru Done

Assumeru commented 1 year ago

I now hope your planned changes involve removing the const_casts from MyGUI_Delegate.h somehow 😛

@Assumeru Done

Awesome.