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

Save bytecode to file through CLI #170

Merged
merged 9 commits into from
Nov 9, 2021
Merged

Save bytecode to file through CLI #170

merged 9 commits into from
Nov 9, 2021

Conversation

NotDSF
Copy link
Contributor

@NotDSF NotDSF commented Nov 7, 2021

This pull request adds support to save script bytecode through the CLI using the --binary option. This feature is very useful for developers who would like to parse luau bytecode using 3rd-party programs, as said in this feature request #151

@zeux
Copy link
Collaborator

zeux commented Nov 7, 2021

Thanks for the PR! A few notes:

  • It would be better to reuse the code for compileFile, as well as the command line argument. I would propose --compile=format where mode can be text or binary, with --compile being equivalent to --compile=text, and a CompileFormat enum argument to compileFile (see Analyze.cpp where we do something similar for reporting). This will make it possible for us to add a different format that supports bulk compilation, e.g. msgpack where we output multiple bytecode blobs, one per each input.
  • We should output data to stdout still; this requires setting file mode for stdout to binary on Windows. That allows the user of the command line tools to redirect the output to the file of choice.
  • Please don't use std::ofstream - we don't use file streams to avoid associated compile time and run time inefficiencies - and use C output facilities like fwrite
  • Please use the code style that matches the style for this project, e.g. { must be on its own line.

@NotDSF NotDSF closed this Nov 7, 2021
@NotDSF NotDSF reopened this Nov 8, 2021
@NotDSF NotDSF closed this Nov 8, 2021
@NotDSF NotDSF reopened this Nov 8, 2021
@NotDSF
Copy link
Contributor Author

NotDSF commented Nov 8, 2021

Thanks for the PR! A few notes:

* It would be better to reuse the code for `compileFile`, as well as the command line argument. I would propose `--compile=format` where mode can be `text` or `binary`, with `--compile` being equivalent to `--compile=text`, and a CompileFormat enum argument to `compileFile` (see Analyze.cpp where we do something similar for reporting). This will make it possible for us to add a different format that supports bulk compilation, e.g. `msgpack` where we output multiple bytecode blobs, one per each input.

* We should output data to stdout still; this requires setting file mode for stdout to binary on Windows. That allows the user of the command line tools to redirect the output to the file of choice.

* Please don't use std::ofstream - we don't use file streams to avoid associated compile time and run time inefficiencies - and use C output facilities like `fwrite`

* Please use the code style that matches the style for this project, e.g. `{` must be on its own line.

I have fixed these issues. You can now use --compile=format instead.

@NotDSF NotDSF closed this Nov 8, 2021
@NotDSF NotDSF reopened this Nov 8, 2021
@NotDSF NotDSF closed this Nov 8, 2021
@NotDSF NotDSF reopened this Nov 8, 2021
@zeux
Copy link
Collaborator

zeux commented Nov 8, 2021

Please don't close this PR. It's not necessary - you can update it without closing - and that results in needless notifications for people watching the repository.

@NotDSF
Copy link
Contributor Author

NotDSF commented Nov 8, 2021

Please don't close this PR. It's not necessary - you can update it without closing - and that results in needless notifications for people watching the repository.

Sorry about that 👍

CLI/Repl.cpp Outdated
printf("%s", bcb.dumpEverything().c_str());
break;
case CompileFormat::Binary:
printf("%s", bcb.getBytecode().data());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use fwrite to stdout since the data is binary

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks! Left a few comments.

CLI/Repl.cpp Outdated
@@ -444,7 +458,32 @@ int main(int argc, char** argv)
return 0;
}

if (argc >= 2 && strcmp(argv[1], "--compile") == 0)
if ((argc >= 2 && strcmp(argv[1], "--compile=text") == 0) || strcmp(argv[1], "--compile") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be simpler if we just have if (argc >= 2 && strncmp(argv[1], "--compile", strlen("--compile")) == 0), and inside the if we can compare the argv to determine the format. That way you get to reuse the rest of the code between two branches.

CLI/Repl.cpp Outdated
@@ -444,7 +458,32 @@ int main(int argc, char** argv)
return 0;
}

if (argc >= 2 && strcmp(argv[1], "--compile") == 0)
if ((argc >= 2 && strcmp(argv[1], "--compile=text") == 0) || strcmp(argv[1], "--compile") == 0)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs something like _setmode(_fileno(stdout), O_BINARY); when _WIN32 is defined, see
https://stackoverflow.com/questions/23107609/is-there-way-to-set-stdout-to-binary-mode

CLI/Repl.cpp Outdated
if (isDirectory(argv[i]))
{
traverseDirectory(argv[i], [&](const std::string& name) {
if (name.length() > 4 && name.rfind(".lua") == name.length() - 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

luau too?

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks! I'll take care of .luau support in a separate change.

@zeux zeux merged commit 3ba0bdf into luau-lang:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants