anniecherkaev (Annie Cherkaev)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 28 2016, 10:43 AM (138 w, 2 d)

Recent Activity

Jul 29 2016

anniecherkaev added a comment to D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176).

Oh! Yes, you're right, my mistake!

Jul 29 2016, 4:45 PM
anniecherkaev added a comment to D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176).

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)))

with

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 29 2016, 8:36 AM

Jul 28 2016

anniecherkaev added a comment to D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176).

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.

Jul 28 2016, 3:13 PM
anniecherkaev updated the diff for D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176).
  • Implemented requested revisions to Trac #12177- Removed unnecessary comment
Jul 28 2016, 3:07 PM
anniecherkaev updated the diff for D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176).
Jul 28 2016, 3:02 PM
anniecherkaev added a comment to D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176).

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.

Jul 28 2016, 1:04 PM
anniecherkaev retitled D2434: Relevant Bindings no longer reports shadowed bindings (fixes #12176) from to Relevant Bindings no longer reports shadowed bindings (fixes #12176).
Jul 28 2016, 11:48 AM