lemire / FastPriorityQueue.js

a fast heap-based priority queue in JavaScript
Apache License 2.0
357 stars 57 forks source link

feat(queue): add remove, clone, forEach functions #13

Closed blaskovicz closed 6 years ago

blaskovicz commented 6 years ago

Implements #11

blaskovicz commented 6 years ago

The pull request failed on 2/4 because of the nodejs version and my usage of some newer js features (let, const, default params, etc). Sort of related to #12 , are you planning to support newer javascript versions or stay backwards compatible? Either way we could use babel or something to transpile during npm/yarn publish.

lemire commented 6 years ago

Please merge with master. We need to assess the performance consequences of this PR. The new travis script ensures that benchmarks are run.

Do you think that let, const, and default params, are worth breaking backward compatibility at this time? Your fork does not run on my Linux servers right now. Not being to run the code on current Ubuntu Linux seems like a big negative.

You can achieve the default parameter effect with a little code duplication.

There is little gain in replacing var by let or const.

I would prefer to wait for the time when fresh servers run default node versions with support for these features.

lemire commented 6 years ago

What is yarn.lock?

lemire commented 6 years ago

You have left a "let" in the code. I think.

lemire commented 6 years ago

Can you remove the yarn file as well?

And then please add your name in the credit.

Sorry to be a pain.

Note: I verified that the perf was not negatively affected.

blaskovicz commented 6 years ago

yarn.lock is yarn's generated dependency file so that dependencies two people checking out a project get the same exact dependency versions. I can remove it still, if you want, but I advise switching from npm to yarn.

blaskovicz commented 6 years ago

do you want me to make a CONTRIBUTORS.md?

blaskovicz commented 6 years ago

added the contributors file, squashed some of the superfluous commits, and updated the README with the existing and new instance methods 👌

lemire commented 6 years ago

I approve of the contrib file!

I see that yarn.lock is to be checked in.

So I guess all is well.

I’ll do a final review soon to be sure it builds everywhere nicely.

blaskovicz commented 6 years ago

I removed the yark.lock when I rebased. Tests pass now so should be good!

lemire commented 6 years ago

One more thing, why did you add an extra force parameter in _percolateUp... This never gets called anyhow and it is clear what purpose it would serve...?

If it is not useful, it is better to remove it.

blaskovicz commented 6 years ago

It's used in remove such that I can avoid the compare function and always bubble an element up so it can be popped / polled from the top.

lemire commented 6 years ago

I'll merge as is, but my question had to do with the fact that _percolateUp is always called with force = true.

lemire commented 6 years ago

Thanks for the contribution.

blaskovicz commented 6 years ago

Thanks for merging. Percolate up was only called with force in remove , not elsewhere since that would break the default compare and ordering behavior. My case was special and I didn't want to fork the function

lemire commented 6 years ago

I have issued a new release which includes your changes. It appears to build and run well everywhere.

blaskovicz commented 6 years ago

Great, thank you! I started using this code today in my lru cache which is why I became interested. I can submit an example code when I have one.

lemire commented 6 years ago

The README.md could benefit form more examples!