Feature/C++ header only wrapper for C API

[figueroa1395]

[figueroa1395]

Very draft version still. NOT review ready.

[figueroa1395]

I will continue resolving the comments tomorrow.

[TonyXiang8787]

@mgovers @figueroa1395 My proposal is to route the existing C-API test through cpp headers, or create new test. Otherwise we are not testing the cpp wrappers.

[TonyXiang8787]

a common comments: you need to call handle_.check_error() in all constructor body of other classes to throw exception if an error occurs in the construction. For other function body you also need to call this.

[TonyXiang8787]

Another general comment:

unique_ptr requires that the custom deleter should be able to receive a nullptr and the deleter in this case should be a no-op. We need to check if all current PGM_destory_* C API functions satisfy this requirement.

[mgovers]

Another general comment:

unique_ptr requires that the custom deleter should be able to receive a nullptr and the deleter in this case should be a no-op. We need to check if all current PGM_destory_* C API functions satisfy this requirement.

good one. if we find any that do not, we should fix that in a separate PR.

[mgovers]

@mgovers @figueroa1395 My proposal is to route the existing C-API test through cpp headers, or create new test. Otherwise we are not testing the cpp wrappers.

yeah testing is one of the remaining tasks.

What do you prefer? rerouting C API tests, or make another test executable that tests the C++ wrapper? (we've been discussing both options and my gut feeling suggested the latter for isolated testing)

[TonyXiang8787]

@mgovers @figueroa1395 My proposal is to route the existing C-API test through cpp headers, or create new test. Otherwise we are not testing the cpp wrappers.

yeah testing is one of the remaining tasks.

What do you prefer? rerouting C API tests, or make another test executable that tests the C++ wrapper? (we've been discussing both options and my gut feeling suggested the latter for isolated testing)

My proposal is direct re-route the current C-API tests. We can remove the test of meta data part (which depends on the header only core). And write a simple meta data test part to test just some attributes.

[figueroa1395]

My proposal is direct re-route the current C-API tests. We can remove the test of meta data part (which depends on the header only core). And write a simple meta data test part to test just some attributes.

I am currently working on adding the tests by re-routing the current C-API tests.

a common comments: you need to call handle_.check_error() in all constructor body of other classes to throw exception if an error occurs in the construction. For other function body you also need to call this.

This has been addressed through the use of the with_call function on each constructor.

[figueroa1395]

I failed to remove the tests efficiently, but the PR is ready for review. I will change it from draft once the tests have been moved to a different PR.

[mgovers]

please also have a look at the sonar cloud warnings. not all of them are relevant, but most of them are (we can also go over them together if you prefer)

[figueroa1395]

@mgovers I addressed all the relevant sonar cloud warnings. It should be okay now.

[figueroa1395]

The implementation is ready for review. The tests are re-routed and refactored in #707.

[sonarqubecloud[bot]]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud