Add Briloop #418
Add Briloop #418
Conversation
gerardogtn
force-pushed
the
briloop-release
branch
from
c484466 to
9e04de7
Compare
gerardogtn
force-pushed
the
briloop-release
branch
from
9e04de7 to
8859a2c
Compare
There was a problem hiding this comment.
Very nice!! This is looking great, and quite self-contained.
I have a few high-level suggestions, both of which could be handled in separate PRs if you prefer:
- Do you have documentation for the control-flow language extension? Let's add that to the language extension docs.
- Do you have any tests for the relooper implementation? It would be great to include those too.
And here are some lower-level suggestions:
- Can we please name the top-level directory
reloop? I've mostly usedbril-*for the generic language libraries rather than for specific tools. Like, if we were to separate out the Bril language utilities from this relooper tool (not saying we should), those would go in a library calledbril-ktor whatever. - Let's add a documentation page for the tool itself. A good place to start would be by moving your existing
README.mdtodocs/tools/reloop.md. - I mentioned this in an inline comment as well, but any chance there's a simple way to adopt an existing Kotlin-friendly build tool (to avoid special-purpose shell scripts for installing dependencies and building the jar)?
There was a problem hiding this comment.
Please remove the jar file (the build product); we'll just keep the source files in the repository.
| ## Installation | ||
| * Run `./install.sh` (requires sudo) will create a symbolic link in /usr/local/bin | ||
|
|
||
| ## Building | ||
| * Run `./setup.sh` to download jar dependencies. | ||
| * Run `./build.sh` to compile the project. | ||
| * Run `./run.sh` to run the transformation. | ||
| * Run `./release.sh` to generate the release jar. |
There was a problem hiding this comment.
Sorry if this is a silly question (I have not been in the JVM ecosystem for a long time), but is there a standard build tool (Gradle or Maven or Amper or something) that we could use here to avoid custom shell scripting? A simple build setup could make this easier for people to use and maintain.
There was a problem hiding this comment.
Looks like this may have snuck in by mistake?
| formatted = map(lambda x: instr_to_string(x, indentLevel), instrs) | ||
| joined = '\n'.join(formatted) | ||
| return joined |
There was a problem hiding this comment.
A generator expression would be a bit shorter and easier to read:
return '\n'.join(instr_to_string(i, indent_level) for i in instrs)|
|
||
|
|
||
| def instr_to_string(instr): | ||
| def formatted_children(instrs, indentLevel): |
There was a problem hiding this comment.
Python variable names are traditionally camel_case, so indent_level.
| loop: 0, // ADDED | ||
| block: 0, // ADDED | ||
| if: 1, // ADDED | ||
| break: 0, // ADDED | ||
| continue: 0, // ADDED |
There was a problem hiding this comment.
We can dispense with the ADDED comments now. :)
| | { "action": "abort"; "label": bril.Ident }; | ||
| | { "action": "abort", "label": bril.Ident } | ||
| | { "action": "break", "level": number } // Break out of n loops. | ||
| | { "action": "continue", "level": number }; // Continue to next iteration of innermost loop. |
There was a problem hiding this comment.
The comment appears out-of-date, because continue also has a level parameter (so it's not just the innermost loop).
2 participants
Add briloop to the bril repo!