PowerGridModel / power-grid-model

Python/C++ library for distribution power system analysis
Mozilla Public License 2.0
143 stars 30 forks source link

[BUG] incorrect use of const senmentics in CPP wrapper of C-API #748

Open TonyXiang8787 opened 4 days ago

TonyXiang8787 commented 4 days ago

The Problem

In the CPP wrapper of the C-API, almost all the classes have a unique_ptr to the opaque pointers returned by the C-API with the custom deleter. This is the standard way of resource management via C-API. However, because of the pointer semantics, the const functions of these classes can also get non-const pointer to the actual resource. This leads to mismatch on what is correct between semantically and syntactically.

For example, the add_buffer function of DatasetMutable receives a Buffer const& data which is extremely confusing to the user. Is the buffer provided mutable or not? Moreover, to make the linter happy, we added // NOSONAR: no-const.

https://github.com/PowerGridModel/power-grid-model/blob/dec5729391298cebcf63b5c9fcdd04bfb1cdbf03/power_grid_model_c/power_grid_model_cpp/include/power_grid_model_cpp/dataset.hpp#L103-L104

Another example is that the update function of Model is const, confusing for the user.

https://github.com/PowerGridModel/power-grid-model/blob/dec5729391298cebcf63b5c9fcdd04bfb1cdbf03/power_grid_model_c/power_grid_model_cpp/include/power_grid_model_cpp/model.hpp#L40

Root cause

The root cause of the problem is that in a const version, you still get a non-const pointer to the actual resource, which is syntactically correct. To make the linter happy, we made many semantically incorrect const functions, or used // NOSONAR: no-const to silence the linter.

Solution

To solve this issue, we need to define two get functions for all the classes.

Customize unique pointer

We customize the get function of the unique pointer. Concretely we change the using declaration:

https://github.com/PowerGridModel/power-grid-model/blob/2487f490a81598c619f7da78d573b03c33ae60d2/power_grid_model_c/power_grid_model_cpp/include/power_grid_model_cpp/basics.hpp#L52-L53

To a customized class:

// unique pointer definition
template <typename T, auto func> 
class UniquePtr: public std::unique_ptr<T, DeleterFunctor<func>> {
  public:
    using std::unique_ptr::unique_ptr;
    using std::unique_ptr::operator=;
    T* get() { return static_cast<std::unique_ptr<T, DeleterFunctor<func>>&>(*this).get(); }
    T const* get() const { return static_cast<std::unique_ptr<T, DeleterFunctor<func>> const&>(*this).get(); }
};

Getter function

We should define two get functions for all the classes, one for const and one for non-const. For example for Model class, we should do the following.

    PowerGridModel* get() { return model_.get(); }
    PowerGridModel const* get() const { return model_.get(); }

This forces that the const object can only return a const pointer.

Re-route internal functions

We re-route all other functions of the same class by using get. This is already the case for most of them, but we need to check for sure.

Fix compilation error

Now if we try to compile the code, many places (in the cross reference of the wrapper class, and in the tests) will fail because the const-correctness. We fix the error by removing the incorrect const declaration in the function arguments, tailing const, or variable declaration.

Remove the linter hack

Now the linter will not complain about the const syntactically, remove all the linter hack for SonarCloud and clang-tidy.