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

cleanup initialization and move operations for pinnable_mapped_file #52

Merged
merged 1 commit into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/chainbase/chainbase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,13 @@ namespace chainbase {
database(const std::filesystem::path& dir, open_flags write = read_only, uint64_t shared_file_size = 0,
bool allow_dirty = false, pinnable_mapped_file::map_mode = pinnable_mapped_file::map_mode::mapped);
~database();

database(database&&) = default;
database& operator=(database&&) = default;

database(const database&) = delete;
database& operator=(const database&) = delete;

bool is_read_only() const { return _read_only; }
void flush();

Expand Down
6 changes: 3 additions & 3 deletions include/chainbase/pinnable_mapped_file.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ class pinnable_mapped_file {
bip::file_lock _mapped_file_lock;
std::filesystem::path _data_file_path;
std::string _database_name;
size_t _database_size;
bool _writable;
bool _sharable;
size_t _database_size = 0;
bool _writable = false;
bool _sharable = false;

bip::file_mapping _file_mapping;
bip::mapped_region _file_mapped_region;
Expand Down
36 changes: 15 additions & 21 deletions src/pinnable_mapped_file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -433,31 +433,25 @@ void pinnable_mapped_file::save_database_file(bool flush /* = true */) {
std::cerr << "CHAINBASE: Writing \"" << _database_name << "\" database file, complete." << '\n';
}

pinnable_mapped_file::pinnable_mapped_file(pinnable_mapped_file&& o) noexcept :
_mapped_file_lock(std::move(o._mapped_file_lock)),
_data_file_path(std::move(o._data_file_path)),
_database_name(std::move(o._database_name)),
_file_mapped_region(std::move(o._file_mapped_region))
pinnable_mapped_file::pinnable_mapped_file(pinnable_mapped_file&& o) noexcept
{
_segment_manager = o._segment_manager;
_writable = o._writable;
_non_file_mapped_mapping = o._non_file_mapped_mapping;
o._non_file_mapped_mapping = nullptr;
o._segment_manager = nullptr;
o._writable = false; //prevent dtor from doing anything interesting
// all members are correctly default-initialized, so we can just move into *this
*this = std::move(o);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this leave items like _segment_manager and _writable still "live" from what is being moved from? The old move constructor nulls these out so the dtor doesn't do stuff to something that has been moved from, but now it seems like it would

Copy link
Contributor Author

@greg7mdp greg7mdp Aug 15, 2024

Choose a reason for hiding this comment

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

I believe this would execute pinnable_mapped_file::operator=(pinnable_mapped_file&& o), which swaps all the members, so o._segment_manager would get the current value of this->_segment_manager which is nullptr as initialized in the member declaration.

Copy link
Member

Choose a reason for hiding this comment

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

okay yeah that makes sense

}

pinnable_mapped_file& pinnable_mapped_file::operator=(pinnable_mapped_file&& o) noexcept {
_mapped_file_lock = std::move(o._mapped_file_lock);
_data_file_path = std::move(o._data_file_path);
_database_name = std::move(o._database_name);
_file_mapped_region = std::move(o._file_mapped_region);
_non_file_mapped_mapping = o._non_file_mapped_mapping;
o._non_file_mapped_mapping = nullptr;
_segment_manager = o._segment_manager;
_writable = o._writable;
o._writable = false; //prevent dtor from doing anything interesting
o._segment_manager = nullptr;
std::swap(_mapped_file_lock, o._mapped_file_lock);
std::swap(_data_file_path, o._data_file_path);
std::swap(_database_name, o._database_name);
std::swap(_database_size, o._database_size);
std::swap(_writable, o._writable);
std::swap(_sharable, o._sharable);
std::swap(_file_mapping, o._file_mapping);
std::swap(_file_mapped_region, o._file_mapped_region);
std::swap(_non_file_mapped_mapping, o._non_file_mapped_mapping);
std::swap(_non_file_mapped_mapping_size, o._non_file_mapped_mapping_size);
std::swap(_db_permissions, o._db_permissions);
std::swap(_segment_manager, o._segment_manager);
return *this;
}

Expand Down