Passing a mutable string reference for a trivial string getter seems like unneeded complexity. D1667 does the same to the JS::Values.
https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
Details
Details
- Reviewers
- None
Test whether we really don't return pointers and that RVO really avoids the copies.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 6437 Build 10663: Vulcan Build Jenkins Build 10662: arc lint + arc unit
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/771/
Comment Actions
@vladislavbelov are most compilers really smart enough to use copy elision for strings here as widely claimed on the internet?
(Some places use one pattern, other places use the other pattern, we should make up our mind what we want, otherwise it sounds contradictory if we claim there to be good reasons for both.)
Comment Actions
Smart, but the (N)RVO isn't guaranteed. Only since C++17.
I agree, that we should have the same style, but we may have different cases. We can easily return a small std::string, but a huge object can't be allocated on a stack.
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 958 | Ternary operator here and below. | |
Comment Actions
https://de.cppreference.com/w/cpp/language/copy_elision looks more like C++11, only some other features C++17?
I agree, that we should have the same style, but we may have different cases. We can easily return a small std::string, but a huge object can't be allocated on a stack.
At least for strings getters like here it seems useless indirection.
But what about JS::Values, like in D1667 - do they also work with copy elision, i.e.
- void XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface, JS::MutableHandleValue ret) + JS::Value XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface)
If copy elision doesnt work with JS::Value, we might eventually change JS::Value JSI_VisualReplay::GetReplays(ScriptInterface::CxPrivate* pCxPrivate, bool compareFiles) and so forth, so that people chose the better variant for new code?
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 958 | I once had to do revert of a ternary in a getter because the if return return pattern should be used everywhere. So the else keyword should go I suppose | |
Comment Actions
Yeah, compilers started optimize these cases a long time ago. But there're many nuances.
But what about JS::Values, like in D1667 - do they also work with copy elision, i.e.
- void XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface, JS::MutableHandleValue ret) + JS::Value XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface)
js/Value.h from SpiderMonkey:
* - Note that JS::Value is 8 bytes on 32 and 64-bit architectures. Thus, on * 32-bit user code should avoid copying jsval/JS::Value as much as possible, * preferring to pass by const Value&.
std::string is ~24bytes usually, so it's not worse to return JS::Value .
So I'd prefer to do some measurements/perf-tests.
Comment Actions
I read a bit more, you are right that copy elision is only encouraged in C++11 and guaranteed only in C++17.
Good news is one can remove even more copies and rely on pointers and references instead of copy elision.
See inline comments, abandoning the patch as it is superseded by D2271 removing further intermediary instead of only changing the function signature.
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 887 | const & | |
| source/lobby/XmppClient.h | ||
| 85 | GetPresence and GetRole return string literals in any case, and they are not translated and only contain ASCII, so we can actually make them return the pointer instead of having rely on copy elision. GetSubject() is converted using wstring_from_utf8 every single call, so instead it can store that wstring once and return a reference, thus avoiding the copy every single time without relying on copy elision. These changes were performed in D2271. GetNick is void XmppClient::GetNick(std::string& nick)
{
nick = m_mucRoom->nick().to_string();
}and is used as: std::string nick; g_XmppClient->GetNick(nick); return wstring_from_utf8(nick); Notice that to_string() creates a new string and copies that into the nick argument. It's not trivial whether the current code nick = m_mucRoom->nick().to_string(); is faster, or not actually slower than m_mucRoom->nick().to_string(); (former possibly doing one more copy). | |