Bug 85671 - Lack of `std::move()` inside `operator/` for `std::filesystem::path`.
Summary: Lack of `std::move()` inside `operator/` for `std::filesystem::path`.
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 8.1.1
: P3 normal
Target Milestone: 7.4
Assignee: Jonathan Wakely
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-06 13:43 UTC by LIU Hao
Modified: 2018-07-04 14:35 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-05-07 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description LIU Hao 2018-05-06 13:43:31 UTC
In 'bits/fs_path.h' there is

```
  /// Append one path to another
  inline path operator/(const path& __lhs, const path& __rhs)
  { return path(__lhs) /= __rhs; }
```


Isn't this returning a copy of the unnamed object, because `operator/=` returns an lvalue reference?  

I think the operand of the return statement should be `move(path(__lhs) /= __rhs)`.
Comment 1 Jonathan Wakely 2018-05-07 12:13:17 UTC
It would be better to do:

  path __tmp(__lhs);
  __tmp /= __rhs;
  return __tmp;

That allows a copy to be elided.
Comment 2 LIU Hao 2018-05-07 14:41:44 UTC
在 2018/5/7 20:13, redi at gcc dot gnu.org 写道:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85671
> 
> Jonathan Wakely <redi at gcc dot gnu.org> changed:
> 
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>               Status|UNCONFIRMED                 |ASSIGNED
>     Last reconfirmed|                            |2018-05-07
>             Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
>       Ever confirmed|0                           |1
> 
> --- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> It would be better to do:
> 
>    path __tmp(__lhs);
>    __tmp /= __rhs;
>    return __tmp;
> 
> That allows a copy to be elided.
> 

There is no difference in implementation as long as NRVO is in effect.
Comment 3 Jonathan Wakely 2018-05-07 17:27:00 UTC
Author: redi
Date: Mon May  7 17:26:28 2018
New Revision: 260009

URL: https://gcc.gnu.org/viewcvs?rev=260009&root=gcc&view=rev
Log:
PR libstdc++/85671 allow copy elision in path concatenation

By performing the /= operation on a named local variable instead of a
temporary the copy made for the return value can be elided.

	PR libstdc++/85671
	* include/bits/fs_path.h (operator/): Permit copy elision.
	* include/experimental/bits/fs_path.h (operator/): Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/fs_path.h
    trunk/libstdc++-v3/include/experimental/bits/fs_path.h
Comment 4 Jonathan Wakely 2018-05-07 17:46:43 UTC
(In reply to Liu Hao from comment #2)
> There is no difference in implementation as long as NRVO is in effect.

There is a difference.

Using move(path(__lhs) /= __rhs) has one copy construction and one non-elided move construction.

What I've committed to trunk has a copy construction and an elided move construction.
Comment 5 Jonathan Wakely 2018-07-04 11:45:36 UTC
Author: redi
Date: Wed Jul  4 11:44:56 2018
New Revision: 262388

URL: https://gcc.gnu.org/viewcvs?rev=262388&root=gcc&view=rev
Log:
PR libstdc++/85671 allow copy elision in path concatenation

By performing the /= operation on a named local variable instead of a
temporary the copy made for the return value can be elided.

Backport from mainline
2018-05-07  Jonathan Wakely  <jwakely@redhat.com>

	PR libstdc++/85671
	* include/bits/fs_path.h (operator/): Permit copy elision.
	* include/experimental/bits/fs_path.h (operator/): Likewise.

Modified:
    branches/gcc-8-branch/libstdc++-v3/ChangeLog
    branches/gcc-8-branch/libstdc++-v3/include/bits/fs_path.h
    branches/gcc-8-branch/libstdc++-v3/include/experimental/bits/fs_path.h
Comment 6 Jonathan Wakely 2018-07-04 11:54:32 UTC
Also fixed for 8.2
Comment 7 Jonathan Wakely 2018-07-04 13:59:41 UTC
Author: redi
Date: Wed Jul  4 13:59:06 2018
New Revision: 262404

URL: https://gcc.gnu.org/viewcvs?rev=262404&root=gcc&view=rev
Log:
PR libstdc++/85671 allow copy elision in path concatenation

By performing the /= operation on a named local variable instead of a
temporary the copy made for the return value can be elided.

Backport from mainline
2018-05-07  Jonathan Wakely  <jwakely@redhat.com>

	PR libstdc++/85671
	* include/experimental/bits/fs_path.h (operator/): Likewise.

Modified:
    branches/gcc-7-branch/libstdc++-v3/ChangeLog
    branches/gcc-7-branch/libstdc++-v3/include/experimental/bits/fs_path.h
Comment 8 Jonathan Wakely 2018-07-04 14:35:00 UTC
And 7.4