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

Add optional source positions #161

Merged
merged 12 commits into from
Feb 4, 2022
Merged

Add optional source positions #161

merged 12 commits into from
Feb 4, 2022

Conversation

sampsyo
Copy link
Owner

@sampsyo sampsyo commented Feb 3, 2022

This adds very simple optional source positions to Bril programs in their native JSON representation. The objects for all AST nodes (functions, labels, instructions…) can optionally include a pos object, which has two keys: row and col. The idea is sort of in service of #155, i.e., giving better error messages in a static type checker. However, the reference interpreter could consider supporting these too for better error messages.

I documented this optional feature in the language reference. I also added support to the bril2json parser: positions are off by default, and a new -p flag enables them. As a bonus, I expanded the docs for the text format to include actual instructions about how to install and use the tools.

Of course, there are no semantics attached to the actual source position values—other front-end compilers beyond bril2json could just as easily associate their source language's positions with Bril AST nodes.

@Pat-Lafon
Copy link
Contributor

Hi,

  • It doesn't look like the const operations in your test case have a pos field?
  • Can a valid program have some json objects with pos fields and others without?
  • I am not sure what the col values in the example test case mean. I'm assuming col is mostly included for the benefit of some other front-end language. Did you consider having bril2json output something like "col" : 0 or however much the indentation is?

@sampsyo
Copy link
Owner Author

sampsyo commented Feb 4, 2022

Thanks for taking a look, @Pat-Lafon! And for pointing out the missing positions on const instructions; I added those.

I updated the docs to clarify a bit: I don't think it should be required that positions be all-or-nothing. They're purely "gravy."

The column should indicate something about where the error came from within a line. It could help disambiguate this program, for example:

@main { v: int = const 42; print v; }

The column for instructions pointed to the opcode. That made sense for effect operations, but it was a little weird for value ops—thanks for pointing that out! I changed it so the position now points to the beginning of the destination variable in value ops (i.e., the beginning of the entire instruction).

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.

2 participants