Skip to content

Consider dropping or limiting use of simulator_index and function_index #611

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

Open
kyllingstad opened this issue Sep 4, 2020 · 2 comments
Labels
discussion needed Let's have a discussion about this enhancement New feature or request

Comments

@kyllingstad
Copy link
Member

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.

@kyllingstad kyllingstad added enhancement New feature or request discussion needed Let's have a discussion about this labels Sep 4, 2020
@hplatou
Copy link
Contributor

hplatou commented Sep 21, 2020

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

@kyllingstad
Copy link
Member Author

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:

  • Keep things as they are, close this issue.
  • Drop the simulator indexes entirely, including from execution.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Let's have a discussion about this enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants