Not supported fix

[karczex]

This change is 

[KFilipek]

Reviewed 3 of 3 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @karczex)

[codecov]

Codecov Report

Merging #904 (2ef7661) into master (50893e9) will increase coverage by 0.43%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
+ Coverage   93.14%   93.57%   +0.43%     
==========================================
  Files          27       27              
  Lines        2947     2943       -4     
==========================================
+ Hits         2745     2754       +9     
+ Misses        202      189      -13     

Impacted Files	Coverage Δ
src/engine.cc	54.54% <33.33%> (+3.03%)	⬆️
src/libpmemkv.cc	92.11% <100.00%> (+1.99%)	⬆️
src/libpmemkv.hpp	91.89% <0.00%> (+0.27%)	⬆️

[igchor]

Reviewed 2 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karczex)

---

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

		add_engine_test(ENGINE stree
			BINARY transaction_basic

I think it would be better to have separate test for NOT_SUPPORTED and run it only for engines which do not support transactions.

[lukaszstolarczuk]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karczex)

---

src/engine.cc, line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2017-2020, Intel Corporation */

2021

---

src/engine.cc, line 216 at r1 (raw file):

internal::transaction *engine_base::begin_tx()
{
	throw internal::not_supported("Transactions are not supported");

if you want to be verbose, make it .. in this engine

---

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

add_test_generic(NAME config_cpp TRACERS none memcheck)

build_test(blackhole_test engines/blackhole/blackhole_test.cc)

we usually extended blackhole_test for C++ API - it should be added I guess (since blackhole also does not support iter/tx)

---

tests/engine_scenarios/transaction/basic.cc, line 1 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause

if you add tests like this for tx, then add it also for iters

---

tests/engine_scenarios/transaction/basic.cc, line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2020, Intel Corporation */

2021

[karczex]

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @igchor, @karczex, @KFilipek, and @lukaszstolarczuk)

---

src/engine.cc, line 2 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

2021

Done.

---

src/engine.cc, line 216 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

if you want to be verbose, make it .. in this engine

Done.

---

tests/engine_scenarios/transaction/basic.cc, line 1 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

if you add tests like this for tx, then add it also for iters

Really good catch :). It appeared, there is another bug in iterators. Done.

---

tests/engine_scenarios/transaction/basic.cc, line 2 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

2021

Done.

[KFilipek]

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

[karczex]

Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)

---

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

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

we usually extended blackhole_test for C++ API - it should be added I guess (since blackhole also does not support iter/tx)

iterators are implemented for blackhole. Done

[lukaszstolarczuk]

Reviewed 4 of 5 files at r2, 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @karczex)

[igchor]

Reviewed 1 of 5 files at r2, 1 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karczex)

---

src/libpmemkv.cc, line 657 at r3 (raw file):

		return PMEMKV_STATUS_INVALID_ARGUMENT;

	try {

Could you just use catch_and_return status here instead of manually catching the exceptions? It'll cover more cases (e.g. OUT_OF_MEMORY)

---

src/libpmemkv.cc, line 672 at r3 (raw file):

		return PMEMKV_STATUS_INVALID_ARGUMENT;

	try {

.

[karczex]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

---

src/libpmemkv.cc, line 657 at r3 (raw file):

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

Could you just use catch_and_return status here instead of manually catching the exceptions? It'll cover more cases (e.g. OUT_OF_MEMORY)

Done.

---

src/libpmemkv.cc, line 672 at r3 (raw file):

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

.

Done.

[igchor]

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @KFilipek and @lukaszstolarczuk)

---

src/libpmemkv.cc, line 672 at r3 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

Done.

Imo it's better to use existing solutions so I think it might be a little bit easier (and less error-prone) to use unique_ptr instead of this cleanup function. It would just look like this:

auto uptr = std::unique_ptr<pmemkv_write_iterator>(new pmemkv_write_iterator());
uptr->iter = iterator_from_internal(...);
*it = uptr.release();
return PMEMKV_STATUS_OK;

[igchor]

Reviewed 1 of 5 files at r2, 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @karczex)

[KFilipek]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karczex)

---

src/libpmemkv.cc, line 668 at r4 (raw file):

		return PMEMKV_STATUS_INVALID_ARGUMENT;

	return catch_and_return_status(__func__, [&] {

PRETTY_FUNCTION also decorate function under GCC. After some preprocessor if's we can get nice error handling
https://www.rowleydownload.co.uk/arm/documentation/gnu/gcc/Function-Names.html

[igchor]

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

---

src/libpmemkv.cc, line 668 at r4 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

PRETTY_FUNCTION also decorate function under GCC. After some preprocessor if's we can get nice error handling
https://www.rowleydownload.co.uk/arm/documentation/gnu/gcc/Function-Names.html

I don't think we want PRETTY_FUNCTION. It adds function signature and I think it's not needed. For a use, it;s enough to know the name of a function.

[igchor]

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

---

src/libpmemkv.cc, line 668 at r4 (raw file):

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

I don't think we want PRETTY_FUNCTION. It adds function signature and I think it's not needed. For a use, it;s enough to know the name of a function.

for a user*

[karczex]

---

src/libpmemkv.cc, line 668 at r4 (raw file):

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

for a user*

We use PRETTY_FUNCTION in tests, as it's a pleace where it matters.