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

C API #427

Merged
merged 4 commits into from
May 4, 2016
Merged

C API #427

merged 4 commits into from
May 4, 2016

Conversation

kripken
Copy link
Member

@kripken kripken commented May 3, 2016

This adds a C API and a test which shows an example of using it. I've never been much of an API designer, apologies for what you're about to see.

The API is generally similar to LLVM's API which is probably the most popular compiler API. However, as we have structured control flow and statements=expressions etc., this API follows that. As a follow-up, though, I intend to add a basic-block oriented API on top of this, which should be more familiar for compilers (and then using relooping internally we can generate structured control flow as needed).

The "hello world" example and its output are the last two files in the diff.


typedef uint32_t BinaryenType;

BinaryenType BinaryenNone(void);
Copy link
Member

@binji binji May 3, 2016

Choose a reason for hiding this comment

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

Seems strange to not just use an enumeration here. Is this to prevent API breakage if the type values change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also for changes, but the main reason is that compilers using the C API might not want to use a C header. For example, Rust is not written in C, so it can't execute the C header to get those enum values. Instead it links with C libraries and calls methods on them using the C ABI.

Copy link
Member

@sunfishcode sunfishcode May 3, 2016

Choose a reason for hiding this comment

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

Another option is a C++11 enum class with an explicit type:

enum class Color : uint32_t
{
    Red, Green, Blue
};

Copy link
Member Author

Choose a reason for hiding this comment

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

That still doesn't help the Rust use case, though? Or did I misunderstand you?

Copy link
Member

Choose a reason for hiding this comment

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

It might make the C++ use case a little nicer while not harming the rust use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Yeah, I agree that might be nicer. There is the gcc bug mentioned in another comment on this PR, though, which worries me.

@binji
Copy link
Member

binji commented May 3, 2016

Looks pretty good. Have you thought about how you'd manipulate the AST after it has been created? Also, personally I'd suggest something shorter than Binaryen for the prefix. But maybe that's just me. :)

@kripken
Copy link
Member Author

kripken commented May 3, 2016

No, I haven't thought much about manipulation yet. Hopefully most compilers won't need it? Probably that's too optimistic ;)

About the prefix, how about "Bin"? Or any other ideas?

@binji
Copy link
Member

binji commented May 3, 2016

No, I haven't thought much about manipulation yet. Hopefully most compilers won't need it? Probably that's too optimistic ;)

Well, if you imagine the users of the API are just producers than yeah, probably not needed. I was imagining users who will want to read a binary module and then do stuff with it.

About the prefix, how about "Bin"? Or any other ideas?

How about BYE :)

@jfbastien
Copy link
Member

About the prefix, how about "Bin"? Or any other ideas?

How about BYE :)

Known would be nice: "the prefix? It is Known."

@kripken
Copy link
Member Author

kripken commented May 3, 2016

How about BYE :)

BYE suggests BYOB to me... so, Bring Your own... Expression? ;)

But I like the shortening, BYE for BinarYEn. Another option might be BYN?

Probably too short, but maybe, B_?

Known would be nice: "the prefix? It is Known."

Definitely the kind of pun I like, but probably too much for a C API ;)

@kripken
Copy link
Member Author

kripken commented May 3, 2016

Or maybe BIN?

@binji
Copy link
Member

binji commented May 3, 2016

I like BYN better. BIN looks like it could be anything, but I can't think of anything else that looks like BYN. But really, I don't think it matters much. Just like with WebAssembly, you could use Binaryen as the prefix and mostly people won't complain. :)

@kripken
Copy link
Member Author

kripken commented May 3, 2016

Yeah, maybe the choice is BYN (or Byn?) vs Binaryen. I'm ok with either. If no one chimes up I suppose we can leave it as Binaryen for now.

Meanwhile I added a kitchen-sink test of the api, which found a few bugs that are now fixed.

@kripken
Copy link
Member Author

kripken commented May 4, 2016

Ok, let's merge this. We can change the name later if there are opinions about it.

@kripken kripken merged commit 5b2adeb into master May 4, 2016
@kripken kripken deleted the c-api-nice branch May 4, 2016 00:14
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.

4 participants