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.