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

Add value_ptr #59

Merged
merged 10 commits into from
Apr 9, 2024
Merged

Add value_ptr #59

merged 10 commits into from
Apr 9, 2024

Conversation

Alex-PLACET
Copy link
Collaborator

@Alex-PLACET Alex-PLACET commented Apr 4, 2024

Fix #57
Implement value_ptr class

@Alex-PLACET Alex-PLACET marked this pull request as ready for review April 4, 2024 12:25
Copy link
Collaborator

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

Formatting: the opening curly braces should go on a new line

include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/array_data.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
test/test_memory.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

I mainly have a minor suggestion.

include/sparrow/memory.hpp Outdated Show resolved Hide resolved
test/test_memory.cpp Outdated Show resolved Hide resolved
@JohanMabille
Copy link
Collaborator

I think test_memory.cpp should also have a copyright.

include/sparrow/memory.hpp Outdated Show resolved Hide resolved
include/sparrow/memory.hpp Outdated Show resolved Hide resolved
template <class T>
class value_ptr {
public:
value_ptr() = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

unique_ptr is constexpr in C++23, I believe this type can also be written all constexpr although it might not work because of the calls to make_unique. Anyway, an attempt to make this type all constexpr would be helpful in the future.

Copy link
Collaborator

@jjerphan jjerphan Apr 5, 2024

Choose a reason for hiding this comment

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

We only can support C++20 unfortunately, isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be (locally) set to constexpr with a macro only defined when compiling with C++23.

@JohanMabille JohanMabille merged commit c938478 into man-group:main Apr 9, 2024
21 checks passed
@Alex-PLACET Alex-PLACET deleted the add_value_ptr branch April 9, 2024 12:15
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.

Implement a value_ptr class
5 participants