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

Make everything an op #39

Merged
merged 55 commits into from Apr 17, 2016
Merged

Make everything an op #39

merged 55 commits into from Apr 17, 2016

Conversation

@samdoshi
Copy link
Collaborator

@samdoshi samdoshi commented Apr 17, 2016

Lots of changes here, the main result being all keys, arrays and variables have been converted to ops.

@pq

This comment has been minimized.

Copy link
Contributor

@pq pq commented on src/teletype.h in b716990 Apr 9, 2016

Maybe more descriptive field names here?

@pq

This comment has been minimized.

Copy link
Contributor

@pq pq commented on src/teletype.h in b716990 Apr 9, 2016

Will there be more types here? I'd be inclined to name it something like process_result_value_type_t (or kind). Or maybe if it's binary, just a has_value bool?

This comment has been minimized.

Copy link
Contributor

@pq pq replied Apr 9, 2016

I realize of course, that I'm jumping in mid stream so feel free to ignore... And in which case, pardon the distraction! :)

@pq

This comment has been minimized.

Copy link

@pq pq commented on src/teletype.c in 945e35b Apr 9, 2016

Do we have max and min handy? I wonder if this would read better as

a = max(a, 63);

... on second thought maybe not and perhaps not idiomatic C besides!

@pq

This comment has been minimized.

Copy link

@pq pq commented on src/teletype.c in 945e35b Apr 9, 2016

More (rookie) questions about idiomatic C: why not if (!first_cmd && !op.returns) { ... ?

This comment has been minimized.

Copy link
Owner Author

@samdoshi samdoshi replied Apr 10, 2016

No reason, I'll probably change it!

@pq

This comment has been minimized.

Copy link

@pq pq commented on src/teletype.c in 945e35b Apr 9, 2016

a += tele_patterns[pn].l; ?

This comment has been minimized.

Copy link
Owner Author

@samdoshi samdoshi replied Apr 10, 2016

Yes..., but that line is really just pre-existing code duplicated. I didn't want to change that just yet. At some point all of the pattern ops need a tidy up.

@tehn tehn merged commit 3a72a42 into monome:master Apr 17, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants