-
Notifications
You must be signed in to change notification settings - Fork 64
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
Conversation
…passed when the output multiport is implemented.
…Port.lf test, and fix the checks in TSConnectionGenerator.kt
…eclaration of Variable to use multiports.
…t for input multiports.
…ne" to improve readability of the generated code.
…k code in TypeScript code generation.
…ad of using = [] and push() to the array.
…-coding the multiport indices to allow flexible handling of multiport connection.
…Spec.toTSCode() for generating code for multiport width.
…the parameterized width for multiports.
…ua-franca into ts-multiport
…ultiports when reactors are nested.
…ify multiports of arrays.
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.
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.
Co-authored-by: Edward A. Lee <[email protected]>
First of all, thank you for the great comments!
I understand that the
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 agree that |
…lify generated code for multiports in TypeScript. (Move the burden of multiport handling from code generator to reactor-ts).
…tiport # Conflicts: # org.lflang/src/lib/c/reactor-c
…ween port() and _connectMulti().
… trigger when b is a bank). Also add BankToReaction.lf to tests.
@@ -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' |
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.
We should eventually change this and import everything from 'reactor-ts'
once we've published it as a package. Let's do this soon.
Relevant PR: lf-lang/reactor-ts#81
Relevant Issue: lf-lang/reactor-ts#57