Fork me on GitHub
#cljs-dev
<
2018-01-05
>
rauh09:01:56

What's the reason that Atom doesn't actually just implement -reset! and -swap! protocols and instead the entire validation/resetting is done in reset! and swap!?

dnolen13:01:19

@rauh so that stuff isn’t locked into the concrete Atom type

dnolen13:01:40

other IAtom implementers get it

rauh13:01:09

That makes a lot of sense! 🙂

dpsutton16:01:35

@dnolen are you looking for patches for merging today?

dpsutton16:01:16

if so I think we've got minimum example, patch, and tests for the letfn bug https://dev.clojure.org/jira/browse/CLJS-1965

dnolen16:01:23

@dpsutton not sure if I’ll get to that today but it’s on my radar

dpsutton16:01:49

sounds good. thanks for the acknowledgement

dnolen16:01:12

@dpsutton just from a quick review of the patch I think it could maybe be a smaller change

dpsutton16:01:39

possibly. I didn't do the patch just the tests and a smaller demonstration

dpsutton16:01:22

i've never made a patch to the compiler so i left what was done as is. happy to chat about how to improve, read comments on jira, or just mull it over if you don't have time

dnolen16:01:17

left a comment on JIRA

dnolen16:01:16

the problem with always wrapping is that you want to know that it will get eliminated

dnolen16:01:28

these run immediate closures aren’t free

dnolen16:01:43

so would prefer it only be one if we have to

dnolen16:01:51

in :expr case we have to, no choice

dnolen16:01:07

at top-level no choice because of scope leak

dnolen16:01:19

but the other cases would like to avoid

dnolen16:01:17

we have code to track function scopes I thought

dpsutton16:01:20

(let [containing-environment {}]
  (letfn [(g [] 3)]
    (defn f3 [] (g)))

  (letfn [(g [] 4)]
    (defn f4 [] (g))))

(deftest letfn-expression-tests
  (is (= (f3) 3))
  (is (= (f4) 4)))
if i'm understanding you these examples should not need to work around the bug. but the tests for these fail

dnolen16:01:57

that case makes me think that letfn should probably just work more intelligently

dnolen16:01:11

g is only important to the user - not the compiler

dnolen16:01:21

we could generate a name to prevent collisions

dpsutton16:01:53

ok. so you like a gensyms type approach?

dpsutton16:01:24

rather than use a closure to keep it inaccessible just prevent collision of names?

dnolen16:01:10

always using closures is a optimization hurdle, don’t like it

dnolen16:01:26

I don’t think we need garbled gensym

dnolen16:01:05

or rather I mean just gensym with user supplied prefix

dpsutton16:01:39

i'm new to the compiler. is there a way to ask for a var name for that function that no one else will ever use?

dnolen16:01:40

the only reason we can’t do what let does already

dnolen16:01:58

hrm wouldn’t this be problem with let too?

dnolen16:01:02

we should check that

dnolen17:01:42

@dpsutton we use gensym for that

dpsutton17:01:54

doesn't look like let is susceptible to this since it suffixes its bindings

dnolen19:01:41

ok so we had to solve this problem before

favila19:01:08

*lexical-renames*?

dnolen19:01:04

pretty sure yeah

dnolen19:01:39

anyways we shouldn’t do anything new here just do what let does

favila19:01:39

so thanks to hoisting, why wouldn't emit-let just work for letfn?

dnolen19:01:03

it probably could?

favila19:01:27

@dpsutton Maybe try this later: (defmethod emit* :letfn [ast] (emit-let ast false)). Perhaps js hoisting can work for us for once.

dnolen19:01:54

@favila maybe the one issue is that :letfn allows recursive refs between fns?

favila20:01:09

I am thinking js hoisting would take care of that

favila20:01:13

I am not 100% sure though

dnolen20:01:21

I mean from ClojureScript compiler point of view

favila20:01:45

that's why we still need :letfn to be a different ast

dnolen20:01:57

this is just emit

favila20:01:59

but I think the generated js is the same?

favila20:01:29

this is just a hunch from looking at generated let and imaging the same for fns

favila20:01:21

the invariants that prohibit clj let from referencing before assignment are enforced before emission; at emission it looks like it would work

mfikes22:01:42

Wow. I was thinking of publishing Coal Mine to Clojars to make it a little easier to use it for regression testing. But now you can just do something like this with the latest command line tools:

{
  :deps {
    org.clojure/clojurescript {:mvn/version "1.9.1007"}
    coal-mine {:git/url "" :rev "HEAD"}
  }
}
Awesome!

Alex Miller (Clojure team)22:01:21

Boom! But don’t use HEAD.

mfikes22:01:39

Yeah, I was wondering about that 🙂

mfikes22:01:30

By the way @alexmiller Coal Mine works for Clojure as well, if you are ever in want of running a Clojure change through a large corpus of code.