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

WIP Feat/cleanup #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

fstamour
Copy link
Contributor

apart from the obvious move into src/ and split, I removed the function "prin1-e-node-to-string" and I might have broken something doing that. I didn't test yet.

I also extracted a function warn-if-dirty

@fstamour
Copy link
Contributor Author

Ok, quick question before I do anything else.

Are you ok with me moving the code into a subfolder (src)?

and, are you ok with me splitting up cluck.lisp?

If so, I'll create 1 or 2 PR just for these changes. And only after I'll try to actually improve the code.

@stylewarning
Copy link
Member

Are you ok with me moving the code into a subfolder (src)?

Yes.

and, are you ok with me splitting up cluck.lisp?

If the split is good, sure.

(:bind
;; emacs is being weird about the indentation here, something specific about :bind
`((,(cadr binding-car) . ,actual-car)))))
(car-match-multiple (binding-car actual-car)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the "label" car-match-multiple was removed because it was not used (and sbcl was complaining)

@fstamour
Copy link
Contributor Author

🐖

Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

just some early comments

src/checks.lisp Outdated
(let ((dirty-enodes-count
(hash-table-count (e-graph-dirty-e-nodes e-graph))))
(unless (zerop dirty-enodes-count)
(warn "The e-graph is dirty! It has ~d dirty nodes."
Copy link
Member

Choose a reason for hiding this comment

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

~D dirty node~:P

(when (and (symbolp tree)
(string= "_" tree))
(warn "Tried to compile a pattern involving a symbol named \"_\" in a package other than CLUCK. This will not be treated as a wildcard!"))
(and (symbolp tree)
Copy link
Member

Choose a reason for hiding this comment

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

keep this a when since warn is effectively side effectful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warn is now inside the and, the code works the same

Unless you're giving me a style warning 😉

Copy link
Member

Choose a reason for hiding this comment

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

yes it's a style warning. AND is for computing values, not having a shortcut for WHEN :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

AND is for computing values, not having a shortcut for WHEN

cries in sh

Copy link
Member

Choose a reason for hiding this comment

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

crosses pointer fingers perpendicularly such that they lie in a plane whose normal is directed toward sh

nil
'no-match))
(unless (eql a b)
'no-match))
Copy link
Member

Choose a reason for hiding this comment

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

don't remove NIL if the value is meaningful. in this case it is since a boolean is being returned.

use WHEN / UNLESS for side effects only


(defun default-merge-car-bound-values-fn (a b id)
(declare (ignore id))
(if (eql a b)
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -9,9 +9,8 @@
(defun hash-table-peek (ht)
(with-hash-table-iterator (next ht)
(multiple-value-bind (has-value-p k) (next)
(if has-value-p
Copy link
Member

Choose a reason for hiding this comment

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

see prev comment

(if (eql (cadr binding-car) actual-car)
nil
'no-match))
(unless (eql (second binding-car) actual-car)
Copy link
Member

Choose a reason for hiding this comment

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

see prev comment

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.

3 participants