Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix EXPT printing #257
Fix EXPT printing #257
Changes from all commits
f378fbb
197db5d
01dd974
6961318
49140af
bb9999c
1dbf7c3
3d5c907
1d4f707
318461a
d8fbad4
56fe7f9
df5bc5e
7e370d4
c3f6424
b168c4c
de79d15
2fcde9c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we don't check for a valid operator here first (likewise just below). We are relying on the fact that the delayed-expression for sure 💯% has a lisp symbol that maps to a valid Quil operator in the CAR and not, say, CL:GET-INTERNAL-RUN-TIME. If lookup fails,
lisp-symbol->quil-infix-operator
will just return NIL.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to throw an error in
l-s->q-i-o
if it tries to lookup a nonexistent key? That seems like a sane enough thing to do to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be possible, though it would complicate the implementation
valid-quil-function-or-operator-p
, which currently just tries all the lookup functions in order.In general, when adding an abstraction layer like this, I like to keep it purely functional if possible and allow the caller to decide how to handle errors if they want to. For example, if parsing fails, you probably want to signal a "parse-error", but in other places, maybe a different error.
I did consider adding another set of functions with the same name but with
-or-error
or-or-lose
or some such appended, and using it here. In this case, that would map tolisp-symbol->quil-infix-operator-or-error
, to make it easy for callers that are not bothered about a custom error condition. I suppose theon-error
variants could allow the caller to pass in the condition they want on failure.Edit: s/are bothered/are not bothered/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyone worried enough they want to see error checking here before merge? I suppose another option is to let the caller specify via keyword argument what behavior they want on lookup failure, i.e.: 1) return nil, 2) default error, 3) custom condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 worries here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mplayer ~/music/bob-marley/three-little-birds.mp3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Classic @appleby
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew. For a second there I was worried no one would get the joke.