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

Reimplement variadic if without recursion #1375

Closed
wants to merge 2 commits into from

Conversation

refi64
Copy link
Contributor

@refi64 refi64 commented Aug 7, 2017

Closes #842. Agaaain.

@gilch How does this look?

@refi64 refi64 requested a review from gilch August 7, 2017 23:08
@gilch
Copy link
Member

gilch commented Aug 8, 2017

I'd also like a test to make sure the wasted assignment isn't in the AST.

You forgot a pair of square brackets in that import #851, but we could avoid the reduce import and avoid re-implementing partition by using two pop/assignments in a loop, which conveniently produces our arguments in the desired reverse order.

(defmacro if [&rest exprs]
  "if with elif"
  (setv exprs (list exprs)
        expansion (if* (% (len exprs) 2)  ; odd?
                    (.pop exprs)
                    `None))
  (while exprs
    (setv expansion  `(~(.pop exprs) ~expansion)
          expansion `(if* ~(.pop exprs) ~@expansion)))
  expansion)

This builds it inside-out just like your reduce. It's more imperative than functional, but we don't have all our functional tools yet it bootstrap.

We should also consider removing if* from the docs to discourage people from nesting it in their own macros.

@gilch
Copy link
Member

gilch commented Aug 8, 2017

I also noticed the tutorial says there's no elif in Hy and you should use cond. We should clarify that if can have "elif" clauses. The doc updates don't have to be in this PR though--just the fix and the test.

@Kodiologist
Copy link
Member

You know, we could also get rid of the if macro, rename if* to if, and make cond a bootstrap macro. There's no real reason to have both cond and multi-branch if in the same language, right?

@Kodiologist
Copy link
Member

We should also consider removing if* from the docs to discourage people from nesting it in their own macros.

Agreed; the starred special forms that are supposed to be internal to Hy shouldn't be documented for users.

@gilch
Copy link
Member

gilch commented Aug 9, 2017

There's no real reason to have both cond and multi-branch if in the same language, right?

Hy's multi-branch if is more like Clojure's cond. Hy's cond is from Common Lisp. They're not the same. I don't want to get rid of if, but I'd consider moving cond to extra.

In Hy, cond has an implicit do after the condition, which is really only good for side effects in the imperative style. In the more typical functional style, it's just extra brackets. You might as well use an explicit do in the rare branches where you need a side effect and avoid the extra brackets everywhere else.

Hy's cond can also have empty branch bodies, in which case the value of the first truthy condition is returned, like an or. That is, the last expression of the branch is the value of the branch, even if it also happens to be the first expression. (Common Lisp also does this.) cond also has no else clause. It always defaults to None. You can mimic one by using a truthy value, like True or :else as the condition in the final branch, but this doesn't compile to a Python else, because the else always has the default None.

@Kodiologist
Copy link
Member

In Hy, cond has an implicit do after the condition, which is really only good for side effects in the imperative style.

And for setv, since there's no let.

@gilch
Copy link
Member

gilch commented Aug 9, 2017

And for setv, since there's no let.

Yup. That's a real pain point. I wish we could have a proper let in Hy. But setv is a side-effect--albeit a localized one when using (ahem) locals. It helps if you put the if inside the setv instead of the other way around.

With the threading macros, unpacking, doto or comp/partial (and sometimes juxt), and small, focused, functions, I've found that I don't actually need let as much as I thought. When I do use a setv, it's usually just one at the top of the function, not in branches. But in the rare case you need it, you can use the do block. It's the exceptional case, not the common one.

@refi64
Copy link
Contributor Author

refi64 commented Aug 11, 2017

@gilch I changed the implementation like you said.

I'd also like a test to make sure the wasted assignment isn't in the AST.

Is this even really possible right now?

@gilch
Copy link
Member

gilch commented Aug 11, 2017

Is this even really possible right now?

I think so. Even at the native Hy level, we can get the AST compilation of a test form using disassemble, which we can inspect programmatically. How much did the AST change between Python versions in a simple if branch? We could also use astor and see if the word elif appears in the string.

@Kodiologist
Copy link
Member

Travis is showing a failure because the Python 3.5 job was canceled before it ran. You can probably get it to run the tests again with something like git commit --amend --reset-author && git push -f. Or just test Python 3.5 locally.

@Kodiologist
Copy link
Member

Is this even really possible right now?

My guess is yes. You can try making a test in test_ast.py that runs can_compile and recursively walks through the output counting the number of ast.Assign nodes.

@refi64 refi64 closed this Aug 12, 2017
@refi64 refi64 reopened this Aug 12, 2017
@refi64
Copy link
Contributor Author

refi64 commented Aug 12, 2017

Let's see if Travis works now...

@Kodiologist
Copy link
Member

Yay, it does.

@Kodiologist
Copy link
Member

I'll take a crack at testing.

@Kodiologist
Copy link
Member

Kodiologist commented Aug 13, 2017

Actually, I don't think it works. (if 1 1 (if 2 (setv x 2) (if 3 3))) and (cond [1 1] [2 (setv x 2)] [3 3]) still use two anon vars, not one. This might be related to how setv now returns None.

@refi64
Copy link
Contributor Author

refi64 commented Aug 13, 2017

I should probably just check the number of elifs...

@Kodiologist
Copy link
Member

@kirbyfan64, are you still working on this?

@refi64
Copy link
Contributor Author

refi64 commented Sep 15, 2017

I mean, I'm not sure what exactly is wrong because of the setv interactions...

@Kodiologist
Copy link
Member

Kodiologist commented Sep 15, 2017

If you haven't already, I would try putting print statements in compile_if to ensure that it's seeing the Hy model tree you're expecting it to see.

@refi64
Copy link
Contributor Author

refi64 commented Sep 15, 2017

Whoops, my test case was wrong. ¯\(ツ)

@Kodiologist
Copy link
Member

It still doesn't seem to work:

$ echo '(cond [1 1] [2 (setv x 2)] [3 3])' | hy2py
if 1:
    _hy_anon_var_2 = 1
else:
    if 2:
        x = 2
        _hy_anon_var_1 = None
    else:
        _hy_anon_var_1 = (3 if 3 else None)
    _hy_anon_var_2 = _hy_anon_var_1

@Kodiologist
Copy link
Member

I'm closing this for inactivity. Please reopen it when you'd like eyeballs again.

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