search

purescript-deprecated / purescript-maps

33 stars 33 forks source link

clean up use of unsafePartial #128

Closed matthewleon closed 6 years ago

matthewleon commented 6 years ago

A bit of code cleanup that restricts use of unsafePartial to where it's really needed.

hdgarrood commented 6 years ago

Thanks, but the reason it's been done this way is to ensure that unsafePartial is only evaluated once, so that we're not constructing and calling extra functions unnecessarily.

matthewleon commented 6 years ago

I thought unsafePartial is optimized away by the compiler?

matthewleon commented 6 years ago

https://github.com/purescript/purescript/commit/3f1a50e626dd9d7f610c6094e20268e8a7fba6a9

matthewleon commented 6 years ago

And even if that weren't the case, I would understand leaving up the way it is, but the usage of unsafePartial in down still is a bit unclear to me.

hdgarrood commented 6 years ago

I suppose it's possible these are actually equivalent, then. Is there a difference in the generated code? With the code the way it is now, it is at least crystal clear that even if the unsafePartial inlining fails for whatever reason, you'll still only evaluate unsafePartial once.

hdgarrood commented 6 years ago

Just FYI, I'm becoming less keen on refactoring changes which don't change behaviour, because I think the maintenance effort (i.e. reviewing, merging, releasing, etc) effort to payoff ratio is quite small compared to other work I could be doing, and also because it makes git blame less useful.

matthewleon commented 6 years ago

Ah, fair enough. That makes sense.

FWIW, the generated code is indeed equivalent. I get what you are saying as regards the change to up, which shifts unsafePartial to the other side of a function arrow, but the change in down simply eliminates unsafePartial entirely.

That said, I understand the hesitation regarding merging of refactoring changes. I was working on some other, more useful changes, of which these were a (tiny) part. Perhaps if those changes make sense, we can include this kind of thing in there.

hdgarrood commented 6 years ago

Ok, thanks, that's good to know. Re down: oops, I didn't notice that! Since unsafePartial isn't actually needed in down then I think that crosses the threshold of usefulness to be worth considering on its own.

hdgarrood commented 6 years ago

Well that's not quite true; I did notice but I failed to consider the implications properly.

matthewleon commented 6 years ago

I've amended the branch accordingly. If you think it's worth incorporating the change to down please feel free to reopen the PR.

hdgarrood commented 6 years ago

GitHub won't let me reopen for some reason, would you mind sending a new PR?