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 bank & multiport support in TypeScript code generator with > 30 multiport tests. #942

Merged
merged 81 commits into from
Mar 11, 2022

Conversation

hokeun
Copy link
Member

@hokeun hokeun commented Feb 6, 2022

Relevant PR: lf-lang/reactor-ts#81
Relevant Issue: lf-lang/reactor-ts#57

hokeun added 21 commits February 3, 2022 16:46
…passed when the output multiport is implemented.
…Port.lf test, and fix the checks in TSConnectionGenerator.kt
…ne" to improve readability of the generated code.
…-coding the multiport indices to allow flexible handling of multiport connection.
…Spec.toTSCode() for generating code for multiport width.
@hokeun hokeun changed the title Add basic multiport support in TypeScript code generator and test (MultiportToPort.lf) to verify the code generation. Add multiport support in TypeScript code generator and 7 multiport tests to verify the implementation. Feb 10, 2022
@hokeun hokeun requested review from edwardalee and lhstrh February 10, 2022 06:57
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.

Looks like a good start to me! I notice that code handling connections has no provision for the interleaved keyword. This will become an issue when banks are supported.

One high level question: This implementation has quite a bit of if (p.isMultiport()) {...} else {...} in the Kotlin code generator. Would it make more sense to be doing these checks in reactor-ts? It seems that this realization is putting much of the work into the code generator, as done by CGenerator, rather than in the runtime. Will this limit the ability to support mutations?

I did not review the changes to reactor-ts, but some look suspicious. Is package-lock.json really supposed to be in the repo? It was removed from .gitignore. What are the __tests__ directories?

Please take all this with a grain of salt because I'm quite unfamiliar with this code.

org.lflang/src/org/lflang/AstExtensions.kt Outdated Show resolved Hide resolved
@hokeun
Copy link
Member Author

hokeun commented Feb 10, 2022

Looks like a good start to me!

First of all, thank you for the great comments!

I notice that code handling connections has no provision for the interleaved keyword. This will become an issue when banks are supported.

I understand that the interleaved keyword should be taken care of. I believe it shouldn't be too difficult to support that with the current design using an Array of in/out ports for multiports. (I can imagine we just use Arrays of reactors for banks and switch how we order the ports/banks.) I also took note in the issue here.

One high level question: This implementation has quite a bit of if (p.isMultiport()) {...} else {...} in the Kotlin code generator. Would it make more sense to be doing these checks in reactor-ts? It seems that this realization is putting much of the work into the code generator, as done by CGenerator, rather than in the runtime. Will this limit the ability to support mutations?

I think I can move some of the generated code to the reactor-ts runtime (maybe as functions or classes), however, some checks might have to stay in the code generator. Let me talk to Marten and try to move those codes to the runtime as much as I can. Thanks!

I did not review the changes to reactor-ts, but some look suspicious. Is package-lock.json really supposed to be in the repo? It was removed from .gitignore. What are the __tests__ directories?

Please take all this with a grain of salt because I'm quite unfamiliar with this code.

I agree that package-lock.json should probably not be in the repo. I think the __tests__ directories are for unit tests of the reactor-ts runtime. I'm not fully aware of why those changes were made since I was just taking the recent commit of the reactor-ts runtime, but I'll try to clean them up.

@lhstrh
Copy link
Member

lhstrh commented Feb 10, 2022

…lify generated code for multiports in TypeScript.

(Move the burden of multiport handling from code generator to reactor-ts).
@hokeun hokeun changed the title Add bank & multiport support in TypeScript code generator with > 20 multiport tests. Add bank & multiport support in TypeScript code generator with > 30 multiport tests. Mar 2, 2022
@@ -51,10 +51,16 @@ class TSImportPreambleGenerator(
const val DEFAULT_IMPORTS = """
|import commandLineArgs from 'command-line-args'
|import commandLineUsage from 'command-line-usage'
|import {Args as __Args, Present, Parameter as __Parameter, State as __State, Variable as __Variable, Read, Triggers as __Triggers, ReadWrite, Write, Action as __Action, Startup as __Startup, Sched, Timer as __Timer, Reactor as __Reactor, Port as __Port, OutPort as __OutPort, InPort as __InPort, App as __App} from './core/reactor'
|import {Reaction as __Reaction} from './core/reaction'
|import {Parameter as __Parameter, Timer as __Timer, Reactor as __Reactor, App as __App} from './core/reactor'
Copy link
Member

Choose a reason for hiding this comment

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

We should eventually change this and import everything from 'reactor-ts' once we've published it as a package. Let's do this soon.

@lhstrh lhstrh merged commit f6e4534 into master Mar 11, 2022
@lhstrh lhstrh deleted the ts-multiport branch March 11, 2022 04:47
@lhstrh lhstrh added the enhancement Enhancement of existing feature label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants