Skip to content

Add BinaryenModuleWriteSExpr to write a module to a string in s-expr format#2106

Merged
kripken merged 6 commits intoWebAssembly:masterfrom
tweag:may-15-binaryen-module-write-sexpr
May 22, 2019
Merged

Add BinaryenModuleWriteSExpr to write a module to a string in s-expr format#2106
kripken merged 6 commits intoWebAssembly:masterfrom
tweag:may-15-binaryen-module-write-sexpr

Conversation

@bollu
Copy link
Copy Markdown
Contributor

@bollu bollu commented May 15, 2019

Closes Issue #2103.

I'd like some helping with adding and running the test case:

$ python check.py test/example/
$ python check.py test/example/c-api-kitchen-sink.c

both try to run the entire test suite. I'd be glad if someone could help me understand how to run a single test case.

@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented May 15, 2019

Squashed commits and pushed all changes in one commit.

bollu added a commit to tweag/asterius that referenced this pull request May 15, 2019
@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented May 16, 2019

how do I stop the WasmPrinter from printing the ANSI escape sequences for colors ?

@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented May 16, 2019

OK, I found Colors.cpp, which only contains Colors::disable and no Colors::enable, or Colors::getStatus. If I wanted to disable the colors for one print call, I presume there is no API for this?

@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented May 16, 2019

I need this kind of API because the interface that I want is to print compiler logs to stdout, while producing debugging files with the wasm s-expressions. As per the current API, since my tty does point to stdout, colors are printed.

I can disable colors, but this will disable it forevermore, which is weird as well.

@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented May 16, 2019

I sent a PR to expose this color API through the raw C bindings: #2111 (edited to insert link)

@kripken
Copy link
Copy Markdown
Member

kripken commented May 16, 2019

Sadly the test runner doesn't have an option to run just one test currently. The suite is short enough that it's often not that bad, but sometimes it can be annoying, yeah. I sometimes temporarily edit the python file to run just the relevant parts I'm working on. We should improve this.

Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks, nice work!

Comment thread src/binaryen-c.h Outdated
Comment thread src/binaryen-c.cpp Outdated
std::stringstream ss;
WasmPrinter::printModule((Module*)module, ss);

const std::string temp = ss.str();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can use const auto here and below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why auto? it's a simple type, and someone new to C++ many not know the type of .str(). I don't see a benefit of using auto. I'm trying to understand the rationale here --- if this is the project style, then by all means, I'll change the code, but I see no advantage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Bump, @kripken :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is clear enough by convention that .str() returns a string, so it is worth saving on the visual complexity of this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK. changed to const auto.

Comment thread src/binaryen-c.cpp Outdated
WasmPrinter::printModule((Module*)module, ss);

std::string out = ss.str();
const int l = out.length() + 1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how about len instead of l?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread test/example/c-api-kitchen-sink.c Outdated
BinaryenModulePrint(module);

// write the s-expr representation of the module.
BinaryenModuleWriteSExpr(module, buffer, 1024);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please test the other new API call as well

Copy link
Copy Markdown
Contributor Author

@bollu bollu May 17, 2019

Choose a reason for hiding this comment

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

Done.

bollu added a commit to tweag/asterius that referenced this pull request May 17, 2019
- Import changes from binaryen PR#2106:
    WebAssembly/binaryen#2106
- Add bindings to `Raw.hs` for the new APIs
- Use new APIs to print s-expressions to a debug file.
@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented May 21, 2019

Done, all changes have been made and test cases pass :)

@kripken
Copy link
Copy Markdown
Member

kripken commented May 22, 2019

Great, thanks!

@kripken kripken merged commit 257a4c5 into WebAssembly:master May 22, 2019
bollu added a commit to tweag/asterius that referenced this pull request Jul 1, 2019
Add support for printing s-expressions
This is useful for debugging.

This patch also fixes some stray links which reflects the migration of TerrorJack/binaryen to tweag/binaryen.

- Import changes from binaryen PR#2106:
    WebAssembly/binaryen#2106
- Add bindings to `Raw.hs` for the new APIs
- Use new APIs to print s-expressions to a debug file.
@TerrorJack TerrorJack deleted the may-15-binaryen-module-write-sexpr branch July 20, 2019 13:06
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