Conversation
treeowl
force-pushed
the
streamline-foldu
branch
2 times, most recently
from
585a118 to
07a581d
Compare
treeowl
force-pushed
the
streamline-foldu
branch
from
07a581d to
7800163
Compare
* As discussed in lspitzner#55, we were recursively constructing `Foldable` dictionaries in unordered folds (for the basic queues, though not the `Prio` ones). Stop doing that. * Add strict left unordered folds and monoidal unordered folds.
treeowl
force-pushed
the
streamline-foldu
branch
from
7800163 to
ce43ab4
Compare
konsumlamm
left a comment
konsumlamm
left a comment
There was a problem hiding this comment.
I can only repeat what I said in #46 (review): please don't just merge non-trivial changes without review.
| foldMapU :: Monoid m => (a -> m) -> MaxQueue a -> m | ||
| foldMapU f (MaxQ q) = Min.foldMapU (f . unDown) q | ||
|
|
||
| -- | /O(n)/. Unordered left fold on a priority queue. |
There was a problem hiding this comment.
You forgot the note about performance.
|
|
||
| -- | /O(n)/. Unordered strict left fold on a priority queue. | ||
| -- | ||
| -- @since 1.4.2 |
There was a problem hiding this comment.
This seems to be the only file you added @since annotations. They should be on all new (exported) functions.
There was a problem hiding this comment.
Yes, you're right. I'll have to fix that up.
I thought I'd addressed your concerns. Let's try to keep this friendly. |
Sorry, I didn't mean to be unfriendly. The person raising the concerns can usually judge best whether or not they've been addressed, so I think it's useful to await a re-review. Imo, a good rule for (non-controversial) PRs would be that it needs at least one explicit approval (from someone else than the author) to get merged. |
2 participants
As discussed in Make sure we're not building unnecessarily large dictionaries #55, we were recursively constructing
Foldabledictionaries in unordered folds (for the basic queues, though not the
Prioones). Stop doing that.Add strict left unordered folds and monoidal unordered folds.