Add C++ API for config

[lukaszstolarczuk]

added new cpp API for config, used in engines Start functions, added test for cpp config, updated some of tests for c config, updated cpp example

---

This change is 

[igchor]

Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)

---

examples/pmemkv_basic_cpp/pmemkv_basic.cpp, line 57 at r1 (raw file):

	/* See libpmemkv_config(3) for more detailed example of config creation */
	LOG("Creating config");
	config *cfg = new config();

Please do not use pointers here - this way you have memory leak. Just create config on stack.

[lukaszstolarczuk]

Reviewable status: 0 of 14 files reviewed, 4 unresolved discussions (waiting on @igchor)

---

examples/pmemkv_basic_cpp/pmemkv_basic.cpp, line 57 at r1 (raw file):

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

Please do not use pointers here - this way you have memory leak. Just create config on stack.

Done.

---

src/libpmemkv.hpp, line 250 at r3 (raw file):

 * @param[in] count The count of elements stored under reference.
 *
 * @return pmem::kv::status of function's outcome

I'm really not sure about this return comment

---

src/engines-experimental/caching.cc, line 131 at r3 (raw file):

			"caching Exception"); // todo propagate start exceptions properly

	if (basePtr->open(subEngine, pmem::kv::config(subEngineConfig)) != status::OK)

I'm not sure why I had to use "pmem::kv::" here (the internal config was used, without it)

---

tests/engines-experimental/caching_test.cc, line 62 at r3 (raw file):

		// TODO
		auto ret = pmemkv_config_from_json(cfg, json.c_str());

I believe we're leaving this (using json) as is for now...?

[lukaszstolarczuk]

@szyrom suggested, that we could perhaps add error handling method here too...

[igchor]

Reviewed 3 of 9 files at r1, 7 of 7 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaszstolarczuk, @return, and @szyrom)

---

src/libpmemkv.hpp, line 140 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

@szyrom suggested, that we could perhaps add error handling method here too...

Or we could create one free function like errormsg (in pmem::kv namespace).

---

src/libpmemkv.hpp, line 205 at r3 (raw file):

inline config::config(config &&other) noexcept
{
	this->_config = other._config;

You should set other._config to nullptr (otherwise, you will have double free). We should also have test for this - (both copy and move construction/assignment)

---

src/libpmemkv.hpp, line 250 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm really not sure about this return comment

Hm, maybe just write "@return pmem::kv::status" and add doxygen for status enum class (describe all statuses as in documentation). In generated docs "pmem::kv::status" will be clickable link to status class description

---

src/libpmemkv.hpp, line 487 at r3 (raw file):

 * of underlying pmemkv_config variable and sets it to nullptr.
 *
 * @return C config struct

handle to pmemkv_config

---

tests/engines-experimental/caching_test.cc, line 62 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I believe we're leaving this (using json) as is for now...?

Right, we are planning to remove this anyway

[codecov]

Codecov Report

Merging #381 into master will increase coverage by 0.78%.
The diff coverage is 89.02%.

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   80.61%   81.39%   +0.78%     
==========================================
  Files          12       12              
  Lines         753      833      +80     
==========================================
+ Hits          607      678      +71     
- Misses        146      155       +9

Impacted Files	Coverage Δ
src/libpmemkv.hpp	88.81% <89.02%> (-0.07%)	⬇️

[ldorau]

Bindings Travis build fails - please fix

[igchor]

Reviewed 3 of 7 files at r4, 4 of 4 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaszstolarczuk, @return, and @szyrom)

---

tests/engines-experimental/caching_test.cc, line 61 at r5 (raw file):

			throw std::runtime_error("Cannot create config");

		// TODO: updte with C++ config API (get rid of json?)

typo

[lukaszstolarczuk]

Reviewable status: 7 of 13 files reviewed, 2 unresolved discussions (waiting on @igchor)

---

src/libpmemkv.hpp, line 140 at r3 (raw file):

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

Or we could create one free function like errormsg (in pmem::kv namespace).

issue filled and to be discussed: #383

---

src/libpmemkv.hpp, line 205 at r3 (raw file):

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

You should set other._config to nullptr (otherwise, you will have double free). We should also have test for this - (both copy and move construction/assignment)

Done.

---

src/libpmemkv.hpp, line 487 at r3 (raw file):

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

handle to pmemkv_config

Done.

---

tests/engines-experimental/caching_test.cc, line 61 at r5 (raw file):

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

typo

Done.

[ldorau]

Reviewed 1 of 5 files at r7.
Reviewable status: 8 of 13 files reviewed, 2 unresolved discussions (waiting on @igchor)

[ldorau]

Reviewed 1 of 5 files at r6, 2 of 5 files at r7.
Reviewable status: 11 of 13 files reviewed, 2 unresolved discussions (waiting on @igchor)

[igchor]

Reviewed 1 of 5 files at r7.
Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @igchor and @lukaszstolarczuk)

---

src/libpmemkv.hpp, line 90 at r7 (raw file):

 */
enum class status {
	OK = PMEMKV_STATUS_OK,		     /**< no error */

why "<" at the beggining of a comment?

[igchor]

Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @igchor)

[lukaszstolarczuk]

@ldorau, I've updated failing binding (nodejs) and it seems the job is working properly now

[igchor]

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