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

Add get_simulator_index to execution #438

Closed
wants to merge 2 commits into from

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Oct 23, 2019

Allow retrieving the simulation_index using a simulator name.
This is simply just a useful function to have.

The implementation and function name in this PR must be rewritten once we decide to go forward with #384, but lets just add this to our API now.

@markaren markaren changed the title Add get_index to execution Add get_simulator_index to execution Oct 24, 2019
@eidekrist
Copy link
Member

What is the use case for this? With an execution you always have a mapping between simulator name and index from either:

  • simulator_index execution::add_slave()
  • std::pair<execution, simulator_map> load_ssp()
  • std::pair<execution, simulator_map> load_cse_config()

@markaren
Copy link
Contributor Author

markaren commented Oct 29, 2019

The use case was originally #426, but it was implemented in another way.

simulator_index execution::add_slave()
std::pair<execution, simulator_map> load_ssp()
std::pair<execution, simulator_map> load_cse_config()

You might find yourself in a situation where you don't have immidate access to these. Also I have never liked the std::pair<execution, simulator_map> way.

I'm fine with this PR not being merged though.

@kyllingstad
Copy link
Member

As a general rule, I think that any PR that adds functionality should be justified with a concrete use case, or there must be some argument for why it is a superior solution (e.g. it removes a known source of error in client code).

This is simply just a useful function to have.

YAGNI. ;)

@markaren
Copy link
Contributor Author

Closed based on feedback. May revisit in the future.

@markaren markaren closed this Oct 29, 2019
@markaren markaren deleted the feature/execution-get-index branch October 30, 2019 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants