Skip to content

Move object in pop method of List. #3116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 20, 2021
Merged

Move object in pop method of List. #3116

merged 2 commits into from
Jul 20, 2021

Conversation

blacktea
Copy link
Contributor

@blacktea blacktea commented Jul 15, 2021 •
edited by henryiii
Loading

I suggest to move object because it should be deleted from vector, and move constructor likely faster than copy constructor.
BTW, If the class hasn't got a move constructor it would be copied.

Suggested changelog entry:

Move object in ``.pop()`` for list.

@Skylion007
Copy link
Collaborator

Skylion007 commented Jul 15, 2021 •
edited
Loading

@blacktea Seems some of the compilers aren't happy about this.

@blacktea
Copy link
Contributor Author

@blacktea Seems some of the compilers aren't happy about this.

Yep. It seems a compiler bug.
Please, look at it: https://godbolt.org/z/8ocjEzs8d
Clang 3.6 warns, Clang 3.7 is happy.
I will change using copy initialization.

Skylion007
Skylion007 approved these changes Jul 20, 2021
View reviewed changes
Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, @rwgk @henryiii any objections?

@Skylion007 Skylion007 requested a review from rwgk July 20, 2021 15:43
rwgk
rwgk approved these changes Jul 20, 2021
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Feeling good enough about it that it doesn't need the Google testing. (And it's small so easy to roll back in the unlikely event that something goes wrong.)

@Skylion007 Skylion007 merged commit 6d5d4e7 into pybind:master Jul 20, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 20, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021
@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk approved these changes

@Skylion007 Skylion007 Skylion007 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blacktea @Skylion007 @rwgk @henryiii @dummytoday