Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMPROVEMENT] Input/output/update data structures as standard layout #365

Closed
mgovers opened this issue Aug 29, 2023 · 4 comments · Fixed by #430
Closed

[IMPROVEMENT] Input/output/update data structures as standard layout #365

mgovers opened this issue Aug 29, 2023 · 4 comments · Fixed by #430
Assignees
Labels
improvement Improvement on internal implementation

Comments

@mgovers
Copy link
Member

mgovers commented Aug 29, 2023

Background

The underlying input, output and update data structures behind the data types retrieved by initialize_array are generated using code generation.

The current implementation uses inheritance to define the relationship between components and the types they implement and are derived from. This keeps the data structures short and readable.

Problem

The downside is that inheritance causes the data types are not guaranteed to obey standard layout. This may result in less efficient data structures, and also makes the code more bug-prone. As an example, #241 was caused by the fact that the data type does not have standard layout.

Proposed solution:

Plain text with implicit reference operator conversion

// Example program
#include <iostream>
#include <limits>


using ID = int;

struct BaseInput { ID id; };
struct NodeInput{
    ID id;
    double u_rated;
    
    // Sonar Cloud warnings about `explicit` keyword and reinterpret_cast should be disabled here
    operator BaseInput&() { return reinterpret_cast<BaseInput&>(*this); }
    operator BaseInput const&() const { return reinterpret_cast<BaseInput const&>(*this); }

};

int main()
{
    NodeInput const node_input{ 5, 10.0 };
    BaseInput const& base = node_input;
    std::cout << base.id << '\n';
}

Other considered options

Concepts

The standard layout solution can be achieved using a plain struct types without references to any base classes. For written code, that would cause a lot of code duplication, but since the code under consideration is generated, this is not an issue.

The notion of the "base" types are enabled by declaring them concepts, which can be tested using static_assert.

template <typename T>
concept base_input_c = requires (T t) {
    { t.id } -> std::same_as<ID&>;
};

template <typename T>
concept node_input_c = base_input_c <T> && requires (T t) {
    { t.u_rated} -> std::same_as<double&>;
};

template <bool sym>
struct NodeInput {
    ID id{};
    double u_rated{};
};

static_assert(base_input_c<NodeInput<true>>);
static_assert(node_input_c<NodeInput<true>>);
static_assert(base_input_c<NodeInput<false>>);
static_assert(node_input_c<NodeInput<false>>);

Constructors of components will then take the following form (and equivalent for their update and output functions):

class Base {
  public:
    explicit Base(base_input_c auto const& input_data): id_{input_data.id} {}

  private:
    ID id_{};
};

class Node: Base {
  public:
    explicit Node(node_input_c auto const& input_data): Base(input_data), u_rated_{input_data.u_rated} {}

  private:
    double u_rated_{};
};

This also will make the initializer list constructors a lot more readable for e.g. testing purposes.
Intellisense does provide suggestions on e.g. the member functions, but only when the type is constraint auto obj, not constraint auto const& obj.

Composition

Another option could be to use composition, e.g. using data types like

struct BaseOutput{
  ID id{};
};
struct DerivedOutput{
  BaseOutput base{};
  int other_field{};
};

but it becomes prohibitively non-trivial to query data fields of underlying types without getters and setters (e.g. we would need transformer_output.line.base.id to query the id field of a transformer).

@mgovers mgovers added the improvement Improvement on internal implementation label Aug 29, 2023
@TonyXiang8787
Copy link
Member

@mgovers To avoid confusion, I would propose to use a fixed suffix for concepts defined in the input/output data. For example *_c, base_input_c.

@mgovers
Copy link
Member Author

mgovers commented Sep 11, 2023

@TonyXiang8787 my intellisese works on the concepts solution. clang-tidy and code sonar running on the other in #376

@mgovers
Copy link
Member Author

mgovers commented Sep 12, 2023

@TonyXiang8787

  • For the implicit reference option
    • Sonar Cloud wants to add the explicit keyword, which requires adding an explicit static_cast at the usage.
    • Sonar Cloud wants to replace the reinterpret_cast which we would have to disable explicitly
  • For the proposed concepts solution:
    • No issues in clang-tidy and Sonar Cloud
    • Intellisense provides member suggestions correctly

@TonyXiang8787
Copy link
Member

@TonyXiang8787

  • For the implicit reference option

    • Sonar Cloud wants to add the explicit keyword, which requires adding an explicit static_cast at the usage.
    • Sonar Cloud wants to replace the reinterpret_cast which we would have to disable explicitly
  • For the proposed concepts solution:

    • No issues in clang-tidy and Sonar Cloud
    • Intellisense provides member suggestions correctly

For implicit cast, we can disable both implicit cast and reinterpret cast warning from SonarCloud.

For concept approach, good that the IDE recognizes.

Please put both options into candidate approaches, we will decide when we are actually doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants