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

Use new argument field names in dispatch function. #74

Merged
merged 1 commit into from
Feb 5, 2015
Merged

Use new argument field names in dispatch function. #74

merged 1 commit into from
Feb 5, 2015

Conversation

kaeluka
Copy link
Contributor

@kaeluka kaeluka commented Feb 5, 2015

The old argument names are x,y,bar -- corresponding to the
encore level names. The new names are f1..fN. This commit
implements the convention in the dispatch function generation.

To make the produced argument structs more pleasant to read, we also
added an Annotated constructor to the CCode type so the structs
can document the fields.

@TobiasWrigstad
Copy link
Contributor

I'm on it

The old argument names are `x`,`y`,`bar` -- corresponding to the
encore level names. The new names are `f1`..`fN`. This commit
implements the convention in the dispatch function generation.

To make the produced argument structs more pleasant to read, we also
added an 'Annotated' constructor to the 'CCode' type so the structs
can document the fields.
@supercooldave
Copy link

This strikes me as a bad idea, since reading the code you've written, it seems (at least in some places) that you already know the encore level names of the fields, and in order to do the mapping properly elsewhere, I again need to know the encore level names of fields.

But perhaps I'm missing something.

@TobiasWrigstad
Copy link
Contributor

@supercooldave I agree. Apparently keeping the good names makes other code too complicated. I have a strong feeling @EliasC will fix this in the future.

@kaeluka
Copy link
Contributor Author

kaeluka commented Feb 5, 2015

@supercooldave We use the names in the places where we have them. We don't have them in expression translation at the moment. Adding them there would be possible in the future but is not an urgent priority.

TobiasWrigstad added a commit that referenced this pull request Feb 5, 2015
Use new argument field names in dispatch function.
@TobiasWrigstad TobiasWrigstad merged commit 168a60a into parapluu:new-ponyrt Feb 5, 2015
@EliasC
Copy link
Contributor

EliasC commented Feb 5, 2015

+1 on everything :)

@kaeluka kaeluka deleted the new-ponyrt branch February 5, 2015 12:00
kaeluka referenced this pull request Feb 6, 2015
Add some text about Git usage
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