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

Added implementation of buffer #1

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Conversation

JohanMabille
Copy link
Collaborator

No description provided.

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 apart from one observation.

Thank you, @JohanMabille.

bool empty() const noexcept;
size_type size() const noexcept;

template <class U = T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to allow conversion of the stored element type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we may need to have some buffer of "raw" memory (i.e. uint8_t) that we need to reinterpret as another type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Se my comment in the implementation.

using size_type = typename base_type::size_type;

xbuffer() = default;
explicit xbuffer(size_type size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the non-defaulted constructors, consider checking if noexcept is usable.

BTW I suspect this type could be constexpr (and could be tested at compile-time too) but maybe lets consider that in another pass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This constructors allocates memory, so I'm not sure we can use noexcept here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Depends on what's used for allocation exactly, for example std::vector can be instantiated in constexpr context in c++20. But otherwise o lets noexcept where it makes sense. My main worry is copy and move operations as they have an impact on the performance when stored in a vector.

~xbuffer();

xbuffer(const xbuffer&);
xbuffer& operator=(const xbuffer&);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same points as previously with assignations

include/xparrow/xbuffer.hpp Outdated Show resolved Hide resolved
include/xparrow/xbuffer.hpp Show resolved Hide resolved
include/xparrow/xbuffer.hpp Outdated Show resolved Hide resolved
include/xparrow/xbuffer.hpp Show resolved Hide resolved
test/test_xbuffer.cpp Outdated Show resolved Hide resolved
test/test_xbuffer.cpp Show resolved Hide resolved
test/test_xbuffer.cpp Show resolved Hide resolved
@JohanMabille JohanMabille force-pushed the buffer branch 2 times, most recently from 1efe73b to 33ac91b Compare February 12, 2024 18:20
const std::size_t size = 4;
buffer_test_type b1(make_test_buffer(size), size);
buffer_test_type b2(make_test_buffer(size), size);
CHECK(b1 == b2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also !=

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😅

Copy link
Collaborator

@Klaim Klaim left a comment

Choose a reason for hiding this comment

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

We can do a noexcept pass later, but it needs to be done at some point.

Other than my remarks about seeing if it works, LGTM

@JohanMabille JohanMabille merged commit ccafa63 into man-group:main Feb 13, 2024
8 checks passed
@JohanMabille JohanMabille deleted the buffer branch February 13, 2024 17:31
@JohanMabille JohanMabille restored the buffer branch February 29, 2024 21:47
@JohanMabille JohanMabille deleted the buffer branch February 29, 2024 21:48
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.

3 participants