[Misc] Use move semantics where possible #148
Conversation
| const Format format, | ||
| std::uint32_t location = 0, | ||
| const SystemValue systemValue = SystemValue::Undefined) | ||
| StringLiteral name, |
There was a problem hiding this comment.
What are the performance benefits here? The constructor's parameter will now invoke a copy constructor, will it not? So the copy constructor of StringLiteral is either invoked at the caller side or at the callee side.
StringLiteral is also very lightweight, as long as the internal string was not dynamically allocated, e.g. when you use StringLiteral s = std::string("A") + "B" instead of the lightweight version StringLiteral s = "AB".
There was a problem hiding this comment.
The constructor's parameter will now invoke a copy constructor, will it not?
It will copy if an lvalue is passed. If an rvalue is passed it will move it into the name local variable of the constructor and then move it again into the name variable of the struct.
I know it looks confusing, but it is what it is. There is an explanation for that on stackoverflow.
There was a problem hiding this comment.
Thanks for the explanation, that's indeed a great improvement. I've been stuck to the pass-by-reference-to-const method for the longest time. I also found another great article explaining this scenario: https://xania.org/202101/cpp-by-value-args
Would you mind also adding <utility> to the include directives in FragmentAttribute.h?
There was a problem hiding this comment.
Would you mind also adding
<utility>to the include directives in FragmentAttribute.h?
Yes, sure.
| semanticIndex { semanticIndex }, | ||
| instanceDivisor { instanceDivisor } | ||
| name { std::move(semanticName) }, | ||
| format { format }, |
There was a problem hiding this comment.
I appreciate you following the same formatting style ❤️ I know it may seem a bit nitpicky.
|
Thanks for your contribution. The last build check only failed because I closed the PR before it could finish. |
No description provided.