Skip to content
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.

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

Replace auto_ptr by unique_ptr #1889

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aroffringa
Copy link
Contributor

I think it's time to start replacing auto_ptr... it's in some of the header files, and auto_ptr is removed in c++20, which means that other projects that use ola can't be compiled in c++20 mode because of ola. It also solves a lot of warnings :).

This pull request solves also all ambiguous constructor std::unique_ptr(NULL) calls and fixes the linting warnings produces after the find-and-replace of (std::)auto_ptr by (std::)unique_ptr. I've also replaced unnecessary calls to release() by move construction/assignment where possible.

aroffringa added a commit to aroffringa/ola that referenced this pull request Aug 3, 2023
The functions std::binary_function and std::mem_fun have been deprecated in C++11, and are removed in C++20. This pull request replaces them with more modern code. Together with OpenLightingProject#1889 this removes all warnings on recent compilers.
Copy link
Member

@DaAwesomeP DaAwesomeP left a comment

Choose a reason for hiding this comment

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

Looks like you caught all of the instances of std::move() and default initializations. I didn't skim through more than what you changed but I assume you would get compile errors if you missed anything.

I also assume there aren't any instances where these need to be std::shared_ptr, but maybe @peternewman has a better idea of that.

@aroffringa
Copy link
Contributor Author

Looks like you caught all of the instances of std::move() and default initializations. I didn't skim through more than what you changed but I assume you would get compile errors if you missed anything.

I also assume there aren't any instances where these need to be std::shared_ptr, but maybe @peternewman has a better idea of that.

Thanks for the review! An auto_ptr cannot be shared by copying it, so a unique_ptr should behave identical and not introduce the requirement for shared_ptrs. I saw places where raw pointers are stored, and a std::shared_ptr might be suitable in some of those cases, but I would not do that in a single pull request, as it changes the semantics (and there are higher chances of bugs).

I saw also quite a lot of places where a std::unique_ptr is released using release(), and the raw pointer is subsequently passed to a function or stored inside a class. Also in those cases, using c++11 there is probably a method to avoid moving around such raw pointers. On the other hand it all works, so why change that.

I saw you approved the pull request, but I can't merge, so someone else would have to do that.

@DaAwesomeP
Copy link
Member

I saw places where raw pointers are stored, and a std::shared_ptr might be suitable in some of those cases, but I would not do that in a single pull request, as it changes the semantics (and there are higher chances of bugs).

Yeah this is what I meant but I agree that should be a separate pull.

I saw you approved the pull request, but I can't merge, so someone else would have to do that.

That would most likely be @peternewman

aroffringa added a commit to aroffringa/ola that referenced this pull request Aug 21, 2023
The functions std::binary_function and std::mem_fun have been deprecated in C++11, and are removed in C++20. This pull request replaces them with more modern code. Together with OpenLightingProject#1889 this removes all warnings on recent compilers.
@DaAwesomeP
Copy link
Member

@peternewman thoughts on this PR?

@aroffringa
Copy link
Contributor Author

@peternewman Would you want to apply these changes at some point or should I throw away the branch?

aroffringa added a commit to aroffringa/ola that referenced this pull request Feb 3, 2024
The functions std::binary_function and std::mem_fun have been deprecated in C++11, and are removed in C++20. This pull request replaces them with more modern code. Together with OpenLightingProject#1889 this removes all warnings on recent compilers.
@kripton
Copy link
Member

kripton commented Feb 26, 2024

@peternewman: I'd also vote for this PR. All those auto_ptr generate quite some compiler warnings by now

Solve also all ambiguous constructor std::unique_ptr(NULL) calls and fix the linting warnings produces after the find-and-replace of (std::)auto_ptr by (std::)unique_ptr.
@aroffringa
Copy link
Contributor Author

@peternewman can you please merge this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants