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.

Sign up for GitHub

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

Fix comparison of different signedness warning #10

Merged
merged 2 commits into from
Apr 20, 2023
Merged

Fix comparison of different signedness warning #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e9d5c7
Fix comparison of different signedness warning
linh2931 Apr 19, 2023
7d32482
Change _revision from int64_t to uint64_t to have cleaner code and wa…
linh2931 Apr 19, 2023
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
4 changes: 2 additions & 2 deletions include/chainbase/chainbase.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ namespace chainbase {
virtual uint32_t type_id()const = 0;
virtual uint64_t row_count()const = 0;
virtual const std::string& type_name()const = 0;
virtual std::pair<int64_t, int64_t> undo_stack_revision_range()const = 0;
virtual std::pair<uint64_t, uint64_t> undo_stack_revision_range()const = 0;

virtual void remove_object( int64_t id ) = 0;

Expand All @@ -189,7 +189,7 @@ namespace chainbase {
virtual uint32_t type_id()const override { return BaseIndex::value_type::type_id; }
virtual uint64_t row_count()const override { return _base.indices().size(); }
virtual const std::string& type_name() const override { return BaseIndex_name; }
virtual std::pair<int64_t, int64_t> undo_stack_revision_range()const override { return _base.undo_stack_revision_range(); }
virtual std::pair<uint64_t, uint64_t> undo_stack_revision_range()const override { return _base.undo_stack_revision_range(); }

virtual void remove_object( int64_t id ) override { return _base.remove_object( id ); }
private:
Expand Down
17 changes: 7 additions & 10 deletions include/chainbase/undo_index.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ namespace chainbase {
bool _apply = true;
};

int64_t revision() const { return _revision; }
uint64_t revision() const { return _revision; }

session start_undo_session( bool enabled ) {
return session{*this, enabled};
Expand All @@ -521,28 +521,25 @@ namespace chainbase {
if( _undo_stack.size() != 0 )
BOOST_THROW_EXCEPTION( std::logic_error("cannot set revision while there is an existing undo stack") );

if( revision > std::numeric_limits<int64_t>::max() )
BOOST_THROW_EXCEPTION( std::logic_error("revision to set is too high") );

if( static_cast<int64_t>(revision) < _revision )
if( revision < _revision )
BOOST_THROW_EXCEPTION( std::logic_error("revision cannot decrease") );

_revision = static_cast<int64_t>(revision);
_revision = revision;
}

std::pair<int64_t, int64_t> undo_stack_revision_range() const {
std::pair<uint64_t, uint64_t> undo_stack_revision_range() const {
return { _revision - _undo_stack.size(), _revision };
}

/**
* Discards all undo history prior to revision
*/
void commit( int64_t revision ) noexcept {
void commit( uint64_t revision ) noexcept {
revision = std::min(revision, _revision);
if (revision == _revision) {
dispose_undo();
_undo_stack.clear();
} else if( static_cast<uint64_t>(_revision - revision) < _undo_stack.size() ) {
} else if( _revision - revision < _undo_stack.size() ) {
auto iter = _undo_stack.begin() + (_undo_stack.size() - (_revision - revision));
dispose(get_old_values_end(*iter), get_removed_values_end(*iter));
_undo_stack.erase(_undo_stack.begin(), iter);
Expand Down Expand Up @@ -897,7 +894,7 @@ namespace chainbase {
rebind_alloc_t<Allocator, node> _allocator;
rebind_alloc_t<Allocator, old_node> _old_values_allocator;
id_type _next_id = 0;
int64_t _revision = 0;
uint64_t _revision = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I was a little concerned this might cause a consensus failure. However, according to https://en.cppreference.com/w/cpp/language/types Range of values section:

Prior to C++20, the C++ Standard allowed any signed integer representation, [...] [including] ones' complement or sign-and-magnitude.

However, all C++ compilers use two's complement representation, and as of C++20, it is the only representation allowed by the standard...

Therefore the memory layout of int64_t and uint64_t are the same in C++20 and should be the same in pre-C++20 if all compilers use two's complement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for digging into this further. As _revision has not reached std::numeric_limits<int64_t>::max() yet, its representation in both int64_t and uint64_t are the same.

uint64_t _monotonic_revision = 0;
uint32_t _size_of_value_type = sizeof(node);
uint32_t _size_of_this = sizeof(undo_index);
Expand Down