Conversation
denisichh
force-pushed
the
dzubarev/k2-fgets-fgetcsv
branch
from
cfd6a0b to
f03373a
Compare
denisichh
force-pushed
the
dzubarev/k2-fgets-fgetcsv
branch
from
f03373a to
2f38858
Compare
apolyakov
requested changes
Feb 3, 2026
Contributor
apolyakov
left a comment
apolyakov
left a comment
There was a problem hiding this comment.
Added comments about mmap
|
|
||
| // ================================================================================================ | ||
|
|
||
| class mmap_resource final : public sync_resource { |
Contributor
There was a problem hiding this comment.
_resourceprefix seems unnecessary- To think about: maybe it should not be a resource
| // ================================================================================================ | ||
|
|
||
| class mmap_resource final : public sync_resource { | ||
| char* ptr{}; |
Contributor
There was a problem hiding this comment.
I think std::byte* suits better here as it's more type safe
| char* ptr{}; | ||
| k2::descriptor m_descriptor{k2::INVALID_PLATFORM_DESCRIPTOR}; | ||
|
|
||
| explicit mmap_resource(char* ptr, k2::descriptor descriptor) noexcept |
|
|
||
| public: | ||
| mmap_resource(mmap_resource&& other) noexcept | ||
| : ptr{std::move(other.ptr)}, |
Contributor
There was a problem hiding this comment.
It's safer to exchange other.ptr with nullptr
| mmap_resource& operator=(mmap_resource&& other) noexcept { | ||
| if (this != std::addressof(other)) { | ||
| std::ignore = close(); | ||
| ptr = other.ptr; |
Contributor
There was a problem hiding this comment.
Same here with exchanging
| static auto mmap(void* addr, size_t length, int32_t prot, int32_t flags, k2::descriptor fd, | ||
| uint64_t offset) noexcept -> std::expected<mmap_resource, int32_t>; | ||
|
|
||
| auto as_ptr() noexcept -> char*&; |
Contributor
There was a problem hiding this comment.
- Why do we need reference here? The method is called
as_ptr, notas_ref_to_ptr - Isn't it safer to return
std::span<std::byte>? Currently, your implementation looses the information about mmap's length which is quite dangerous
| k2::descriptor descriptor{k2::INVALID_PLATFORM_DESCRIPTOR}; | ||
| const auto ptr{k2::mmap(std::addressof(descriptor), addr, length, prot, flags, fd, offset)}; | ||
| if (ptr == nullptr) [[unlikely]] { | ||
| return std::unexpected{k2::errno_enodev}; |
Contributor
There was a problem hiding this comment.
What do you think about returning k2::einval instead?
| if (m_descriptor == k2::INVALID_PLATFORM_DESCRIPTOR) [[unlikely]] { | ||
| return std::unexpected{k2::errno_enodev}; | ||
| } | ||
| k2::free_descriptor(std::exchange(m_descriptor, k2::INVALID_PLATFORM_DESCRIPTOR)); |
Contributor
There was a problem hiding this comment.
Let's also write nullptr to ptr. It may help in debugging
| class file : public sync_resource { | ||
| bool m_eof{}; | ||
| k2::descriptor m_descriptor{k2::INVALID_PLATFORM_DESCRIPTOR}; | ||
| std::optional<kphp::fs::mmap_resource> m_mmap_resource; |
Contributor
There was a problem hiding this comment.
nit: rename to m_opt_mmap?
|
|
||
| auto descriptor() const noexcept -> k2::descriptor; | ||
|
|
||
| auto has_mmap_resource() const noexcept -> bool; |
Contributor
There was a problem hiding this comment.
I don't see a need for that as you already have a method returning expected<mmap*, int32_t>. Also, isn't it better to have it returning just std::optional<std::reference_wrapper<mmap>>?
scutuatua-crypto
approved these changes
Feb 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.