-
Notifications
You must be signed in to change notification settings - Fork 24
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
[handshake-sim] Added complete modularity for execution models #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments for code style, but more like general recommendations than direct changes.
I did not really look into how the execution models are working internally since I do not have the time (I understood at some point in the past but forgot) but more at the general way you implemented execution models.
All in all I really like it, I think it's going in a great direction! Perhaps sometime early next week we can see the progress and define some kind of milestone at which point we will be able to merge this. Thanks!
experimental/include/experimental/tools/handshake-simulator/ExecModels.h
Show resolved
Hide resolved
Kind of unavoidable to some extent (I think I see what boilerplate you're referring to), don't worry too much at this point.
In general I would be relatively opposed to fail-safe behaviors i.e., as the program if you provide me with a configuration file that does not exist I would rather scream at you in an error message than proceed with a default configuration (which the user might believe is the custom config they specified and then be confused about the results, or worse, take the results for granted). However, if you're just saying "if the user provides no explicit config, use the default one", then yes I totally agree.
Sounds great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nit-picky things to address + a few "real" remarks with easy fixes, but otherwise this looks good!
experimental/include/experimental/tools/handshake-simulator/ExecModels.h
Outdated
Show resolved
Hide resolved
experimental/include/experimental/tools/handshake-simulator/ExecModels.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fixes, this looks good to me!
I still have fix to change, which I'm doing this week. Here are my concerns and stuff to do:
A bit of 'boilerplate' and similar code (struct definition & declaration...)Some functions doesn't have an 'execute' function, but removing the virtual status of it makes the struct without 'execute' abstract, so impossible to store in our map/instanciateMight want can get rid of the tryExecute+execute system of the default simulator which isn't very clear and modular-friendly (Have to redesign a bit more then!)Might want to centralize models map and structures so that users only code in one file