open-simulation-platform / libcosim

OSP C++ co-simulation library
https://open-simulation-platform.github.io/libcosim
Mozilla Public License 2.0
61 stars 10 forks source link

Consider dropping or limiting use of simulator_index and function_index #611

Open kyllingstad opened 4 years ago

kyllingstad commented 4 years ago

I've started wondering if we should switch to using plain pointers rather than simulator_index and function_index – if not in execution, then perhaps in algorithm, observer and manipulator. For example, rather than

algorithm::add_simulator(simulator_index, simulator*, duration)

we'd have

algorithm::add_simulator(simulator*, duration)

Why?

This is C++, after all, and a pointer is just an integer. Implementers of the interfaces I referred to earlier can just as easily map a pointer to whatever internal state they're maintaining for each entity as an integer. That is,

struct my_simulator_data {
    simulator* ptr;
    // other data fields
};
std::map<simulator_index, my_simulator_data> data;

is no better than – and slightly more verbose than –

struct my_simulator_data {
    // data fields
};
std::map<simulator*, my_simulator_data> data;

In other words, we never lose anything in terms of performance or usability by switching to pointers. But we may gain something. A class that doesn't maintain any internal per-simulator or per-function state of its own, and only needs direct access to the objects themselves at various events, currently has to keep a mapping like

std::map<simulator_index, simulator*> simulators;

to obtain that access. That's both more code and an unnecessary additional lookup.

Why (perhaps) not in execution, then?

The idea behind the indexes was to make it clear to users of execution that they shouldn't shoot themselves in the foot by messing around with simulator and function objects themselves, especially not after they've been added to an execution. So you pass those objects to execution and you get a plain, non-dangerous integer back, which you thereafter use consistently to refer to the entities when calling execution functions.

I still think there's a value in that, and I even wish we could encapsulate it even more. But that's a topic for a different issue.

Algorithms, observers, and manipulators, on the other hand, are supposed to mess around with those objects, and the integer overlay doesn't buy them anything.

hplatou commented 4 years ago

Very well explained! I totally agree and will vote for such a change.

kyllingstad commented 4 years ago

When re-reading it now, I see a flaw in my own argumentation. There is one use case for indexes that I've missed, namely when you want to refer to an entity when interacting with an observer/manipulator.

execution exe;
auto obs = make_some_observer();
auto slave = make_some_slave();
exe.add_observer(obs);
auto simIdx = exe.add_slave(slave);

// Here's the use case:
obs->do_something_with_simulator(simIdx);

Two solutions to this:

Of course, there could be some new and improved design that I'm not thinking of.