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.

Sign up for GitHub

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

Jump to bottom

Refactoring of part of the federated package #1663

Merged
merged 23 commits into from
Mar 20, 2023
Merged

Refactoring of part of the federated package #1663

merged 23 commits into from
Mar 20, 2023

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Mar 18, 2023 •
edited
Loading

Fixes #1602. Other things done in this PR:

  • removal of LinkedHashMap<String, Object> federationRTIProperties
  • no passing around of TargetDecl in cases where only Target is needed
  • initialization of TargetConfig in its constructor
  • handling of creating target configurations for federates in a FedTargetConfig subclass
  • checks for target/OS support not scattered but in a single place
  • Fix running RTI on remote host #1559 (but no answer regarding testing)
  • tests

@lhstrh lhstrh mentioned this pull request Mar 18, 2023
Closed
@lhstrh lhstrh mentioned this pull request Mar 19, 2023
Closed
@lhstrh lhstrh marked this pull request as ready for review March 19, 2023 06:36
@lhstrh lhstrh requested a review from edwardalee March 19, 2023 06:37
@lhstrh lhstrh added the refactoring Code quality enhancement label Mar 19, 2023
@lhstrh lhstrh requested a review from petervdonovan March 19, 2023 06:53
edwardalee
edwardalee approved these changes Mar 19, 2023
View reviewed changes
Copy link
Collaborator

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

This looks great!! This is a much needed pass on this code. I've made some really minor suggestions. This looks to me like it may bring us really close to multilingual federates! Maybe we just need to add a mixed target.

org.lflang/src/org/lflang/TargetConfig.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/TargetConfig.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/TargetConfig.java Show resolved Hide resolved
org.lflang/src/org/lflang/federated/generator/FedFileConfig.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/federated/launcher/CBuildConfig.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/federated/launcher/RtiConfig.java Show resolved Hide resolved
lhstrh and others added 3 commits March 19, 2023 11:24
@lhstrh @edwardalee
Update org.lflang/src/org/lflang/TargetConfig.java
dec8e9a 
Co-authored-by: Edward A. Lee <eal@eecs.berkeley.edu>
@lhstrh @edwardalee
Update org.lflang/src/org/lflang/federated/generator/FedFileConfig.java
f3ecec2 
Co-authored-by: Edward A. Lee <eal@eecs.berkeley.edu>
@lhstrh @edwardalee
Update org.lflang/src/org/lflang/federated/launcher/CBuildConfig.java
3203987 
Co-authored-by: Edward A. Lee <eal@eecs.berkeley.edu>
@lhstrh
Copy link
Member Author

lhstrh commented Mar 19, 2023

This looks to me like it may bring us really close to multilingual federates!

Yes, we're getting closer... Let me jot down a design proposal before going ahead and implementing it. The goal of this PR is just to get to the point of having an understandable handling of target configurations.

@lhstrh lhstrh merged commit ab3e0c5 into master Mar 20, 2023
@lhstrh lhstrh deleted the fed-target-conf branch March 20, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwardalee edwardalee edwardalee approved these changes

@petervdonovan petervdonovan Awaiting requested review from petervdonovan

Assignees
No one assigned
Labels
refactoring Code quality enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tracing option fails to appear in targetConfig
2 participants
@lhstrh @edwardalee