Copilot commented on code in PR #843:
URL: https://github.com/apache/incubator-graphar/pull/843#discussion_r2780435026
##########
cpp/src/graphar/status.h:
##########
@@ -154,11 +154,11 @@ class Status {
}
/** Returns a success status. */
- inline static Status OK() { return Status(); }
+ inline static Status OK() { return {}; }
template <typename... Args>
static Status FromArgs(StatusCode code, Args... args) {
- return Status(code, util::StringBuilder(std::forward<Args>(args)...));
+ return Status(code,
std::move(util::StringBuilder(std::forward<Args>(args)...)));
Review Comment:
`std::move` on the temporary returned by `util::StringBuilder(...)` is
counterproductive here: it forces materialization of the temporary and can
inhibit copy elision into the by-value `Status` constructor parameter,
potentially adding an extra move (and for SSO strings, possibly a copy).
Passing the return value directly should be at least as efficient and keeps the
intent clearer.
```suggestion
return Status(code, util::StringBuilder(std::forward<Args>(args)...));
```
##########
cpp/src/graphar/status.h:
##########
@@ -154,11 +154,11 @@ class Status {
}
/** Returns a success status. */
- inline static Status OK() { return Status(); }
+ inline static Status OK() { return {}; }
template <typename... Args>
static Status FromArgs(StatusCode code, Args... args) {
Review Comment:
`FromArgs` takes its parameter pack by value (`Args... args`), which defeats
the perfect-forwarding used by `IOError/KeyError/...` and can introduce extra
copies of the formatting arguments. Consider changing it to `Args&&... args`
and forwarding those into `util::StringBuilder` to preserve the callers’ value
categories.
```suggestion
static Status FromArgs(StatusCode code, Args&&... args) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
