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 cl-quil's tools for safe resolution of included files #224

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

notmgsk
Copy link
Member

@notmgsk notmgsk commented Dec 18, 2019

Apologies for this late PR. This removes some code duplication by using directly cl-quil's functions for safe resolution of include files.

@notmgsk notmgsk requested a review from a team as a code owner December 18, 2019 18:30
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

Won't this require a cl-quil dependency bump?

Comment on lines 70 to 73
(let ((val (loop :for (from to) :in graph
:sum (logxor (ldb (byte 1 from) cut)
(ldb (byte 1 to) cut)))))
val))
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

porque?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Ignore that.

@appleby
Copy link
Contributor

appleby commented Dec 18, 2019

Won't this require a cl-quil dependency bump?

Ah. Looking at test failures, it would appear so.

Copy link
Contributor

@jmbr jmbr left a comment

Choose a reason for hiding this comment

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

Nothing egregiously wrong jumps at me.

(loop :for (from to) :in graph
:sum (logxor (ldb (byte 1 from) cut)
(ldb (byte 1 to) cut))))
(let ((val (loop :for (from to) :in graph
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the rationale for wrapping the accumulated value of the loop in a let form only to return it immediately.

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 should've paid more attention to what I was committing.

@notmgsk
Copy link
Member Author

notmgsk commented Dec 18, 2019

Won't this require a cl-quil dependency bump?

Dang. Yes.

@stylewarning stylewarning merged commit ca7cfca into master Dec 18, 2019
@stylewarning stylewarning deleted the feature/use-quil-safe-resolution branch December 18, 2019 19:05
@notmgsk notmgsk mentioned this pull request Dec 18, 2019
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