Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

radix: adjust implementation to work with basic_dram_inline_string #1021

Merged
merged 2 commits into from Jul 23, 2021

Conversation

igchor
Copy link
Contributor

@igchor igchor commented Jul 19, 2021


This change is Reviewable

@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #1021 (5ced45d) into master (0f79db4) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
+ Coverage   92.63%   92.70%   +0.06%     
==========================================
  Files          37       37              
  Lines        3776     3771       -5     
==========================================
- Hits         3498     3496       -2     
+ Misses        278      275       -3     
Impacted Files Coverage Δ
src/engines-experimental/radix.cc 93.47% <100.00%> (+0.15%) ⬆️
src/engines-experimental/radix.h 96.15% <100.00%> (+1.92%) ⬆️

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @igchor)


src/engines-experimental/radix.h, line 340 at r3 (raw file):

	};

	static_assert(sizeof(queue_entry<uvalue_type>) == 16, "");

you can add some message, instead of empty ""


src/engines-experimental/radix.h, line 449 at r3 (raw file):

template <typename KeyValueType>
const KeyValueType &heterogeneous_radix::queue_entry<KeyValueType>::key() const

what does this do? (why is it this +1 ? )


src/engines-experimental/radix.h, line 456 at r3 (raw file):

template <typename KeyValueType>
const KeyValueType &heterogeneous_radix::queue_entry<KeyValueType>::value() const

+ add a short description, as well

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)


src/engines-experimental/radix.h, line 340 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you can add some message, instead of empty ""

Done.


src/engines-experimental/radix.h, line 449 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

what does this do? (why is it this +1 ? )

Done.


src/engines-experimental/radix.h, line 456 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

+ add a short description, as well

Done.

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @igchor)

Copy link

@karczex karczex left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @igchor)


src/engines-experimental/radix.h, line 318 at r4 (raw file):

	using pmem_type = internal::radix::pmem_type<container_type>;
	using uvalue_type = pmem::obj::experimental::inline_string;
	using dram_uvalue_type = pmem::obj::experimental::basic_dram_inline_string<char>;

You may use defined

pmem::obj::experimental::dram_inline_string

Copy link
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @karczex and @lukaszstolarczuk)


src/engines-experimental/radix.h, line 318 at r4 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

You may use defined

pmem::obj::experimental::dram_inline_string

Done.

@igchor igchor merged commit 15190c5 into pmem:master Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants