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

Minor redundant code or possible bug in instruction (not (not %))? #267

Open
erp12 opened this issue Jan 13, 2019 · 3 comments
Open

Minor redundant code or possible bug in instruction (not (not %))? #267

erp12 opened this issue Jan 13, 2019 · 3 comments

Comments

@erp12
Copy link
Contributor

erp12 commented Jan 13, 2019

I can't be exactly sure what this instruction is meant to do based on its name alone, but something seems off. Seems wrong to have (not (not ...)) anywhere.

(define-registered 
  code_null
  ^{:stack-types [:code :boolean]}
  (fn [state]
    (if (not (empty? (:code state)))
      (push-item (let [item (first (:code state))]
                   (not (not (and (seq? item) (empty? item))))) ;; <-- Seems redundant.
                 :boolean
                 (pop-item :code state))
      state)))

Link to full code:
https://github.com/lspector/Clojush/blob/master/src/clojush/instructions/code.clj#L386

@lspector
Copy link
Owner

lspector commented Jan 14, 2019

(not (not ...)) can be useful to turn a truthy value into an actual true and a falsey value into an actual false:

  (not (not 'truthy!))
=> true
   (not (not nil))
=> false

I haven't investigated whether seq? or empty? can return something that is truthy/falsey rather than actual true/false, but if it can then the and here could as well, and the double not will ensure that we're only pushing an actual true or false onto the :boolean stack.

The code_null instruction should push true onto the :boolean stack if () is on top of the :code stack, and false otherwise.

@erp12
Copy link
Contributor Author

erp12 commented Jan 14, 2019

Gotcha. I am not sure either about seq? and empty?, but I do think that boolean is probably a more clear way to express this.

=> (boolean (and :a :b))
true
=> (boolean (and :a '()))
false

Not crucial to make any changes because behavior is correct.

@erp12 erp12 closed this as completed Jan 14, 2019
@thelmuth
Copy link
Collaborator

I'm reopening this, since it seems at the least this could use a comment to explain the strange code for future generations, and it wouldn't be a bad idea to just change to boolean as @erp12 suggests.

@thelmuth thelmuth reopened this Jan 14, 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

No branches or pull requests

3 participants