hashmap: move reserve() from base class to public method

[lukaszstolarczuk]

This change is 

[codecov]

Codecov Report

Merging #937 (46f0862) into master (2df10e8) will increase coverage by 0.23%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
+ Coverage   95.70%   95.93%   +0.23%     
==========================================
  Files          48       48              
  Lines        6291     6279      -12     
==========================================
+ Hits         6021     6024       +3     
+ Misses        270      255      -15     

Flag	Coverage Δ
tests_clang_debug_cpp17	95.50% <92.30%> (+0.28%)	⬆️
tests_gcc_debug	92.55% <90.90%> (+0.21%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ude/libpmemobj++/container/concurrent_hash_map.hpp	94.54% <92.30%> (+1.11%)	⬆️
include/libpmemobj++/condition_variable.hpp	78.57% <0.00%> (+2.38%)	⬆️
include/libpmemobj++/mutex.hpp	86.20% <0.00%> (+6.89%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[igchor]

Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved

[igchor]

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

---

include/libpmemobj++/container/concurrent_hash_map.hpp, line 2865 at r1 (raw file):

	void
	reserve(size_type buckets)
	{

Hm, I.m wondering if this function should start a tx internally (to make sure that all allocations are within the same tx). It would make allocations faster and it will be more consistent with other modifiers methods behavior.

However, the question is: why rehash() function cannot be called within tx? Should we change that also, or this is because of some internal implementation limitations? @vinser52

If we make those functions transactional we should have tests for rollback (using assert_tx_abort or similar).

[igchor]

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

---

include/libpmemobj++/container/concurrent_hash_map.hpp, line 2865 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Hm, I.m wondering if this function should start a tx internally (to make sure that all allocations are within the same tx). It would make allocations faster and it will be more consistent with other modifiers methods behavior.

However, the question is: why rehash() function cannot be called within tx? Should we change that also, or this is because of some internal implementation limitations? @vinser52

If we make those functions transactional we should have tests for rollback (using assert_tx_abort or similar).

After looking at the code, I guess there is a problem with mark_rehashed which does not snapshot is_rehashed field. But I think, we could just use conditional_add_to_tx there.

[KFilipek]

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk)

---

include/libpmemobj++/container/concurrent_hash_map.hpp, line 1305 at r2 (raw file):

	 */
	void
	reserve(size_type buckets)

If this logic is never used in base class I would to leave empty method and add override specifier in derivate class.
Next thing, is any case when we can create concurrent_hash_map_base? If not maybe we should it mark as pure virtual or make unable to construct base class object.