Change array modifiers (assignment operators, swap and fill) behaviour.

[igchor]

This change is 

[pmem-bot]

@szyrom and @grom72 please review this pull request

[codecov]

Codecov Report

Merging #168 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   92.56%   92.67%   +0.11%     
==========================================
  Files          28       28              
  Lines        1507     1530      +23     
==========================================
+ Hits         1395     1418      +23     
  Misses        112      112

Flag	Coverage Δ
#tests_clang_debug_cpp17	92.78% <100%> (+0.14%)	⬆️
#tests_clang_release	91.63% <100%> (+0.18%)	⬆️
#tests_gcc_debug	94.74% <100%> (+0.08%)	⬆️
#tests_gcc_release_cpp17	91.77% <100%> (+0.26%)	⬆️
#tests_gcc_release_cpp17_no_pmemcheck	91.76% <100%> (+0.27%)	⬆️

Impacted Files	Coverage Δ
include/libpmemobj++/experimental/array.hpp	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3145a5...7adc006. Read the comment docs.

[szyrom]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @igchor)

---

include/libpmemobj++/experimental/array.hpp, line 620 at r1 (raw file):

	/**
	 * Check whether object is on pmem and return pool_base instance.
	 */

@throw

---

tests/CMakeLists.txt, line 240 at r1 (raw file):

build_test(array_modifiers array_modifiers/array_modifiers.cpp)
add_test_generic(array_modifiers none)
add_test_generic(array_modifiers pmemcheck)

memcheck test won't hurt (we have arrays on stack)

---

tests/array_modifiers/array_modifiers.cpp, line 41 at r1 (raw file):

#include <libpmemobj++/make_persistent.hpp>
#include <libpmemobj++/pool.hpp>
#include <libpmemobj++/transaction.hpp>

redundant includes (transaction, pool)

---

tests/array_modifiers/array_modifiers.cpp, line 89 at r1 (raw file):

	try {
		stack_array.fill(1.0);
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 97 at r1 (raw file):

	try {
		stack_array.swap(*(r->ptr_a));
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 105 at r1 (raw file):

	try {
		stack_array = *(r->ptr_a);
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 113 at r1 (raw file):

	try {
		stack_array = std::move(*(r->ptr_a));
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 121 at r1 (raw file):

	try {
		stack_array.swap(stack_array);
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 129 at r1 (raw file):

	try {
		stack_array = stack_array;
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 139 at r1 (raw file):

		stack_array = std::move(ref);
		UT_ASSERT(0);

UT_FATAL()

---

tests/array_modifiers/array_modifiers.cpp, line 146 at r1 (raw file):

Quoted 6 lines of code…

		pmem::obj::transaction::run(pop, [&] {
			pmem::obj::delete_persistent<array_type>(r->ptr_a);
			pmem::obj::delete_persistent<array_type>(r->ptr_b);
		});
	} catch (std::exception &e) {
		UT_FATALexc(e);

delete persistent arrays before stack tests

[igchor]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 620 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

@throw

Done.

---

tests/CMakeLists.txt, line 240 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

memcheck test won't hurt (we have arrays on stack)

Done.

---

tests/array_modifiers/array_modifiers.cpp, line 41 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

redundant includes (transaction, pool)

why redundant? I require pool to call create() and I do start a transaction.

---

tests/array_modifiers/array_modifiers.cpp, line 89 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

UT_FATAL requires string parameter (which UT_ASSERT supplies).

---

tests/array_modifiers/array_modifiers.cpp, line 97 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

.

---

tests/array_modifiers/array_modifiers.cpp, line 105 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

.

---

tests/array_modifiers/array_modifiers.cpp, line 113 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

.

---

tests/array_modifiers/array_modifiers.cpp, line 121 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

.

---

tests/array_modifiers/array_modifiers.cpp, line 129 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

.

---

tests/array_modifiers/array_modifiers.cpp, line 139 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

UT_FATAL()

.

---

tests/array_modifiers/array_modifiers.cpp, line 146 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

		pmem::obj::transaction::run(pop, [&] {
			pmem::obj::delete_persistent<array_type>(r->ptr_a);
			pmem::obj::delete_persistent<array_type>(r->ptr_b);
		});
	} catch (std::exception &e) {
		UT_FATALexc(e);

delete persistent arrays before stack tests

Done.

[szyrom]

Reviewed 2 of 3 files at r2.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @szyrom)

---

tests/array_modifiers/array_modifiers.cpp, line 41 at r1 (raw file):

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

why redundant? I require pool to call create() and I do start a transaction.

array includes transaction and pool

[igchor]

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @szyrom)

---

tests/array_modifiers/array_modifiers.cpp, line 41 at r1 (raw file):

Previously, szyrom (Szymon Romik) wrote…

array includes transaction and pool

Yes, but I think it's better to rely on explicit includes than the fact that array includes them. But I can change this if you want.

[szyrom]

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @szyrom)

---

tests/array_modifiers/array_modifiers.cpp, line 41 at r1 (raw file):

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

Yes, but I think it's better to rely on explicit includes than the fact that array includes them. But I can change this if you want.

ok, let's leave it

[szyrom]

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

[marcinslusarz]

Reviewed 1 of 5 files at r1, 1 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor)

---

include/libpmemobj++/experimental/array.hpp, line 138 at r2 (raw file):

	operator=(const array &other)
	{
		auto pop = _get_pool();

Please move it after the check.

---

include/libpmemobj++/experimental/array.hpp, line 164 at r2 (raw file):

	operator=(array &&other)
	{
		auto pop = _get_pool();

.

---

include/libpmemobj++/experimental/array.hpp, line 551 at r2 (raw file):

	swap(array &other)
	{
		auto pop = _get_pool();

.

[igchor]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @marcinslusarz)

---

include/libpmemobj++/experimental/array.hpp, line 138 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please move it after the check.

I added comment why this is the first thing.

---

include/libpmemobj++/experimental/array.hpp, line 164 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

.

.

---

include/libpmemobj++/experimental/array.hpp, line 551 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

.

.

[marcinslusarz]

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @igchor, @marcinslusarz, and @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 138 at r2 (raw file):

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

I added comment why this is the first thing.

Typo in consistency. Also: consistency has another meaning in context of pmem, so I think this comment needs to be rephrased.

[igchor]

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @marcinslusarz and @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 138 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Typo in consistency. Also: consistency has another meaning in context of pmem, so I think this comment needs to be rephrased.

Done.

[marcinslusarz]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @igchor and @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 640 at r3 (raw file):

		auto pop = pmemobj_pool_by_ptr(this);
		if (pop == nullptr)
			throw pool_error("Invalid pool handle.");

"Object outside of pmemobj pool" or something like that

[igchor]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @marcinslusarz and @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 640 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

"Object outside of pmemobj pool" or something like that

I don't like it either but I copied it from vector to be consistent. We should probably change it globally (in another PR?)

[marcinslusarz]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @igchor and @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 640 at r3 (raw file):

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

I don't like it either but I copied it from vector to be consistent. We should probably change it globally (in another PR?)

The other one - sure. But you are adding new code here - there's no reason not to fix it.

[igchor]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @marcinslusarz and @szyrom)

---

include/libpmemobj++/experimental/array.hpp, line 640 at r3 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

The other one - sure. But you are adding new code here - there's no reason not to fix it.

Done.

[marcinslusarz]

Please add a note to array class description that it must be placed on pmem.

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

[igchor]

Done.

Reviewable status: complete! all files reviewed, all discussions resolved

[marcinslusarz]

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

[kkajrewicz]

Reviewed 2 of 5 files at r1, 2 of 2 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @igchor)

---

include/libpmemobj++/experimental/array.hpp, line 47 at r6 (raw file):

#include <libpmemobj++/experimental/contiguous_iterator.hpp>
#include <libpmemobj++/experimental/slice.hpp>
#include <libpmemobj++/persistent_ptr.hpp>

fix typo in msg of first commit: transction

---

include/libpmemobj++/experimental/array.hpp, line 680 at r6 (raw file):

	 */
	pool_base
	_get_pool()

const

---

tests/array_modifiers/array_modifiers.cpp, line 95 at r6 (raw file):

	array_type stack_array;

add comment why tests below should throw exception; consider separate TC

---

tests/array_modifiers/array_modifiers.cpp, line 147 at r6 (raw file):

		array_type &ref = stack_array;

		stack_array = std::move(ref);

which assignment throws?

---

tests/array_modifiers/array_modifiers_0.cmake, line 5 at r6 (raw file):

#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions

pls remove this file, it's redundant now:)

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 113 at r6 (raw file):

	if (r->ptr->array[0] == 1) {
		auto it = 1;

rather 'val', 'i' or sth like that, imo 'it' is misleading and suggests iterator; or just use int type instead of auto:)

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 114 at r6 (raw file):

	if (r->ptr->array[0] == 1) {
		auto it = 1;
		for (const auto &e : r->ptr->array)

e is just int so there is no gain from avoiding copying; you can ignore this if you like:)

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 117 at r6 (raw file):

			UT_ASSERT(e == it++);
	} else {
		auto it = 2;

.

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 118 at r6 (raw file):

	} else {
		auto it = 2;
		for (const auto &e : r->ptr->array)

.

[igchor]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @kkajrewicz)

---

include/libpmemobj++/experimental/array.hpp, line 47 at r6 (raw file):

Previously, kkajrewicz wrote…

fix typo in msg of first commit: transction

Done.

---

include/libpmemobj++/experimental/array.hpp, line 680 at r6 (raw file):

Previously, kkajrewicz wrote…

const

Done.

---

tests/array_modifiers/array_modifiers.cpp, line 95 at r6 (raw file):

Previously, kkajrewicz wrote…

add comment why tests below should throw exception; consider separate TC

Done.

---

tests/array_modifiers/array_modifiers.cpp, line 147 at r6 (raw file):

Previously, kkajrewicz wrote…

which assignment throws?

The first one is not an array assignment, it;s just creating reference to array (calling array = std::move(array) creates compiler warning, hence I need to use indirection).

---

tests/array_modifiers/array_modifiers_0.cmake, line 5 at r6 (raw file):

Previously, kkajrewicz wrote…

pls remove this file, it's redundant now:)

Done.

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 113 at r6 (raw file):

Previously, kkajrewicz wrote…

rather 'val', 'i' or sth like that, imo 'it' is misleading and suggests iterator; or just use int type instead of auto:)

Done.

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 114 at r6 (raw file):

Previously, kkajrewicz wrote…

e is just int so there is no gain from avoiding copying; you can ignore this if you like:)

Done.

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 117 at r6 (raw file):

Previously, kkajrewicz wrote…

.

Done.

---

tests/array_slice_pmreorder/array_slice_pmreorder.cpp, line 118 at r6 (raw file):

Previously, kkajrewicz wrote…

.

Done.

[kkajrewicz]

Reviewed 5 of 5 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved