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

Cleanup Reactions language #42

Merged
merged 27 commits into from
Aug 3, 2022
Merged

Cleanup Reactions language #42

merged 27 commits into from
Aug 3, 2022

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Jul 27, 2022

This PR cleans up the Reactions language, removes unused classes and functionality, simplifies the runtime classes and the generated code. It particularly affects the runtime project but also improves the language itself.

Major changes are:

General:

  • Cleanup of unused methods in runtime classes and code generators

Runtime:

  • Unification of naming for Reactions runtime classes: We had IReactionRealization, AbstractReactionsRoutineRealization etc. before.
  • Addition of interfaces for all Reactions runtime classes and exchange of dependencies of other classes to those interfaces.
  • Removal of CorrespondenceFailHandler: We had different handlers for failing correspondence retrievals, but they were not integrated properly (they were not exchangeable), such that always an exception was thrown anyway. Since we currently have no need for that functionalty, it is removed rather than integrated.
  • Removal of RoutinesFacadeExecutionState by separation of passing the execution context of a Reaction throughout the execution process (i.e., we only have ReactionsExecutionState now) and tracing the call stack of Reactions and routines.
  • Simplification of instantiating routines facades and passing a ReactionsExecutionState throughout the execution.
  • Removal of ReactionsExecutor: We had redundancy between the ReactionsExecutor and a ChangePropagationSpecification for each Reactions segment. Now only the ChangePropagationSpecification remains

Language:

  • Removal of operation registry from Reactions code generator: Methods are now explicitly created rather than implicitly putting them implicitly into a map upon creation and adding them to a class later on
  • Improval of naming for routines facade classes: They are now named according to the Reactions segment they belong to rather than having a generic name.

@HeikoKlare HeikoKlare force-pushed the cleanup-reactions-language branch 3 times, most recently from e8e731d to c975ab0 Compare August 1, 2022 13:36
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can judge it looks good!

Comment on lines +32 to +35
// generic return type for convenience; the requested type has to match the type of the facade provided during construction
protected def <T extends AbstractRoutinesFacade> T getRoutinesFacade() {
return routinesFacade as T
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we make a generic class to have this explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a quite complicated thing. I will have another look it at, because I am not sure anymore whether this is conceptually necessary or just to not bloat up the code generation as we then have to add the generic type parameter basically everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: We document that point in an issue. Then I will either address that issue in a subsequent PR or document in that issue why we cannot/should not change that, as soon as I have checked it. If this can be improved, it would further bloat up this PR and reviewing the further changes will become even harder than it was for the current changes. Do you agree, @JanWittler?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #46.

Co-authored-by: Jan Wittler <jan.wittler@kit.edu>
@HeikoKlare HeikoKlare merged commit 62e3827 into main Aug 3, 2022
@HeikoKlare HeikoKlare deleted the cleanup-reactions-language branch August 3, 2022 12:15
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.

None yet

3 participants