- User Since
- Jul 28 2016, 10:43 AM (151 w, 4 d)
Jul 29 2016
Oh! Yes, you're right, my mistake!
Hey- I don't know if it's too late or too much hassel to change anything now, but I just thought of the solution I was looking for yesterday- replace that 5 line remove_shadowing function:
reverse $ fst $ foldl (\(bindingAcc, seenNames) binding -> if (occName binding) `elemOccSet` seenNames -- if we've seen it then (bindingAcc, seenNames) -- skip it else (binding:bindingAcc, extendOccSet seenNames (occName binding)))
nubBy (\x y -> (occName x) == (occName y)) bindings
I reran the tests, and they all still pass, I'm pretty sure it's the same behavior. I also realize that since I was reversing the list (so that the inner-most bindings remain first) in that top implementation the performance was actually probably n^2 log n... And this way with nubBy it would be n^2. I can generate a new diff if you'd like, or just leave it be, please let me know what you think would be best.
Jul 28 2016
I implemented your requests, but I have 2 questions: 1) I'm not clear on what the naming convention is- should I be using camel case or underscores? Is it that underscores are used in local functions? What about variable names? 2) I'm not sure that my implementation of the 'remove_shadowing' function is the best approach- I think since I'm passing through an occSet of the names we've seen so far that it should now be n log(n). Please let me know if you think there's a cleaner implementation.
- Implemented requested revisions to Trac #12177- Removed unnecessary comment
- Implemented requested revisions to Trac #12177
Thanks for the review! I'll implement your suggestions and upload a diff in just a bit. I'll add the HasOccName instance to TcIdBinder- that sounds like a more elegant solution.