The Wayback Machine - https://web.archive.org/web/20210306223941/https://github.com/WebAssembly/binaryen/pull/3637
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

wasm-autogen tool #3637

Draft
wants to merge 13 commits into
base: main
from
Draft

wasm-autogen tool #3637

wants to merge 13 commits into from

Conversation

@kripken
Copy link
Member

@kripken kripken commented Mar 2, 2021

This implements one possible way of autogenerating C API code. In the future the JS API could also do this.

The approach taken here is:

  • A C++ tool, wasm-autogen is created. It uses wasm-delegates-fields to emit .h and .cpp code for the C API.
  • Those files are checked in as *.autogen.h / *.autogen.cpp. You can see them in this PR, implementing the Struct operations. (Note: that is after clang-format has been run on them.)
  • Then the C API code is just normal code, and could be tested etc. (Note: I didn't quite finish writing a test for this in this PR, let me know if that would help the discussion.)

I am actually not that happy with this. I feel like a Python script could be simpler and cleaner. One issue is the hacks - both macros and templates - in order to do what amounts to string processing. Look at the code and tell me what you think... Another issue is that iteration on this tool is slightly annoying - imagine that you are in an intermediate step where wasm-autogen emits bad C API code. Then building wasm-autogen will fail (to avoid that, we'd need to get wasm-autogen to not depend on libbinaryen.so which includes the C API, perhaps splitting it up or such).

Instead, I suspect it would be nicer to parse wasm-delegates-fields from Python and do things there. We had a debate about this and decided to try to do more in C++; I think the above paragraph provides some new reasons to avoid C++ there.

cc @aardappel @tlively @aheejin @dschuff @dcodeIO

Side issue, this reorders one wasm-builder method for consistency, as found by starting to autogenerate things.

// ";" at the end.
template<typename T> void autogenOneCAPIDecl(bool startOfImpl = false) {
MixedArena allocator;
T curr(allocator);

This comment has been minimized.

@kripken

kripken Mar 2, 2021
Author Member

This creates an instance of the class as a hack, so that we can use the delegates code on it.

@sbc100
Copy link
Member

@sbc100 sbc100 commented Mar 2, 2021

nit: autogen is a pretty generic term. If there is a tool it should probably include the term "API" or interface somewhere .. something like -api-gen perhaps?

@dcodeIO
Copy link
Collaborator

@dcodeIO dcodeIO commented Mar 2, 2021

Looks great! That'll certainly help a lot with all the boilerplate. I don't have a strong opinion on whether to use C++ or Python for it. As long as it is easy to maintain and makes our lives easier in the long run, that's fine for me. Regarding Python: One needs Python anyway to do anything of significance with the code base, like updating or running tests. Seems rare to generate something new and then don't test it.

@tlively
Copy link
Member

@tlively tlively commented Mar 3, 2021

I came in expecting to strongly prefer a Python approach, but actually this looks pretty reasonable to me. It's certainly not beautiful code, but I don't think the Python version would be either, especially since it would have to parse the delegation header as well. I would be happy to be proven wrong about that, though!

Copy link
Contributor

@aardappel aardappel left a comment

Agree with @tlively that this looks like an uncomplicated amount of C++, and that having it in C++ has maintenance advantages (as discussed before). But I guess a lot of that is a cultural issue.

std::cout << "BINARYEN_API BinaryenExpressionRef\n" \
<< "Binaryen" << #id << "(BinaryenModuleRef module";

#define DELEGATE_FIELD_CHILD(id, name) \

This comment has been minimized.

@aardappel

aardappel Mar 4, 2021
Contributor

why are these macros? looks like some inline functions would not be that much more verbose?

This comment has been minimized.

@tlively

tlively Mar 4, 2021
Member

This lets them all refer to their shared context, which is useful because theses are usually used to generate fragments of a single larger function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants