-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor/shrink modernize scopedtimer (Alternative to #13236) #13258
Refactor/shrink modernize scopedtimer (Alternative to #13236) #13258
Conversation
src/util/timer.h
Outdated
#endif | ||
m_maybeTimer = std::make_optional<Timer>(std::move(strKey), compute); | ||
// | ||
auto assembledKey = QString(std::forward<T>(key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is unfortunately not for free, because the compiler decides to create a local String copy:
https://godbolt.org/z/67M4MTc6j
Here is the `const QString&``version:
https://godbolt.org/z/9WbMedoej
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in both cases, it still holds that capacity() == 0
, thus the copy is not heap-allocated, so I don't really see a problem. Anyways, wdyt of the fixup. It avoids the construction of the temporary by use of an IILE.
3636289
to
46c7faa
Compare
46c7faa
to
4ece08b
Compare
Ready for review @daschuer. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you.
Thank you. |
#13236 but squashed and on top of main