Fix issue #322 #333
Fix issue #322 #333
Conversation
There was a problem hiding this comment.
I see the failure log from #322, but @vinser52 can you describe why/what was wrong?
Reviewable status: 0 of 1 files reviewed, all discussions resolved
I have passed wrong parameter to constructor. When we are creating a new node, the last parameter should be previous head. But in internal_copy() method I passed b->tmp_node by mistake. |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
Codecov Report
@@ Coverage Diff @@
## master #333 +/- ##
==========================================
+ Coverage 95.88% 96.11% +0.23%
==========================================
Files 32 32
Lines 2986 2986
==========================================
+ Hits 2863 2870 +7
+ Misses 123 116 -7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
Wrong parameter is passed to constructor. When we create a new node, the last parameter should be previous head. But in internal_copy() method the b->tmp_node is passed by mistake. Assert caught the issue on Windows. The assert is raised only when more than one elements fit to the same bucket when we do internal_copy().
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vinser52)
include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2953 at r1 (raw file):
allocate_node_copy_construct( pop,
It still fails, another issue was raised while running the test, see updated #322
vinser52
force-pushed
the
fix_internal_copy
branch
from
a58c03e
to
57c0751
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @kkajrewicz)
include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2953 at r1 (raw file):
Previously, kkajrewicz (Krzysztof Kajrewicz) wrote…
It still fails, another issue was raised while running the test, see updated #322
Done.
There is a mistake in the unit test itself. The test make wrong assumption that keys are ordered in the concurrent_hash_map. But concurrent_hash_map is unordered associative container
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @kkajrewicz)
There was a problem hiding this comment.
Fix typo in commit msg: s/cocnurrent/concurrent
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2953 at r1 (raw file):
Previously, vinser52 (Sergei Vinogradov) wrote…
Done.
There is a mistake in the unit test itself. The test make wrong assumption that keys are ordered in the concurrent_hash_map. But concurrent_hash_map is unordered associative container
OK. Now it works
There is a mistake in the unit test itself. The test make wrong assumption that keys are ordered in the concurrent_hash_map. But concurrent_hash_map is unordered associative container.
vinser52
force-pushed
the
fix_internal_copy
branch
from
57c0751
to
8266729
Compare
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
This change is