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

Refactor doGenerate in C generator and parts of Python and Typescript docker generators #1141

Merged
merged 23 commits into from
May 7, 2022

Conversation

housengw
Copy link
Contributor

@housengw housengw commented May 5, 2022

Changes:

  • Use local variable in doGenerate instead of changing topLevelName
  • Get rid of class variable topLevelName
  • Factored out runCommand global variable from CGenerator
  • clean up unused imports
  • Move all docker file related functions and variables away from GeneratorBase, CGenerator, and PythonGenerator into a dedicated code generator class (DockerGeneratorBase.java)

@housengw housengw force-pushed the refactor-c-generator branch from cea4ddf to 25e3891 Compare May 5, 2022 04:01
@housengw housengw marked this pull request as ready for review May 7, 2022 00:07
@housengw housengw requested a review from Soroosh129 May 7, 2022 00:07
@housengw housengw changed the title Refactor c generator Refactor doGenerate in C generator and parts of Python and Typescript docker generators May 7, 2022
@housengw housengw requested a review from lhstrh May 7, 2022 00:20
@housengw
Copy link
Contributor Author

housengw commented May 7, 2022

Currently fileConfig is reassigned to an instance of FedFileConfig inside CGenerator.doGenerate and PythonGenerator.doGenerate in the case of federated execution. This reflects the assumption that fileConfig.srcGenPath can point to different locations at different places of doGenerate, which is a bad design. I took several attempts at getting rid of this design pattern but failed because of the ubiquity of code that depend on that assumption (mostly in federated code, ex. CCompiler, CCmakeGenerator, ...) as well as my lack of understanding of the compile process.

@Soroosh129
Copy link
Contributor

The reasoning behind this reassignment is simple: we want each federate to have its own src-gen folder. If you get rid of this overriding, all federates will be put in a single folder.

The latter used to be the original behavior for federated programs and this overriding of fileConfig was a patchwork to get the new (folderalized?) behavior.

The overwritten fileConfig should only be effective inside doGenerate in CGenerator within the for(FederateInstance federate:federates) loop. I think there are only a handful of things that need fileConfig there. For example, copying necessary files to the src-gen folder needs to know that each federate has a separate folder, etc.

@lhstrh
Copy link
Member

lhstrh commented May 7, 2022

I agree with @housengw that reassigning the fileConfig and topLevelName class variable is is a bad practice that makes the code base unnecessarily brittle. There are other ways of configuring the output behavior of code generators that are more modular.

@Soroosh129
Copy link
Contributor

I agree with @housengw that reassigning the fileConfig and topLevelName class variable is is a bad practice

Right. As I said in my previous comment, there should only be a handful of places that are affected by the fileConfig reassignment, so I would expect it to be relatively straightforward to fix.

@lhstrh
Copy link
Member

lhstrh commented May 7, 2022

I got the sense that @housengw could use some help with that. Would you be able to take a stab at it?

@Soroosh129
Copy link
Contributor

I got the sense that @housengw could use some help with that. Would you be able to take a stab at it?

Let’s see if my comment above helps @housengw first :)

@housengw
Copy link
Contributor Author

housengw commented May 7, 2022

I think this PR has reached a good stopping point. I will address @Soroosh129's comment in a future PR.

Copy link
Member

@lhstrh lhstrh 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! Another step into the right direction :-) I only had some minor nitpicks regarding comments.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@housengw housengw merged commit 2a96627 into master May 7, 2022
@housengw housengw deleted the refactor-c-generator branch May 7, 2022 23: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.

3 participants