roc-streaming / roc-toolkit

Real-time audio streaming over the network.

https://roc-streaming.org

Mozilla Public License 2.0

1.02k stars 204 forks source link

HashMap implementation extract #611

Closed nolan-veed closed 8 months ago

nolan-veed commented 8 months ago

Why

For https://github.com/roc-streaming/roc-toolkit/issues/579

What

Carefully pulled out the code from templated class to implementation class header and cpp file.
Implementation class has no idea/concept of ownership, etc. - so that lives in the main class.
Implementation class has no idea about key equality comparison - so provided that as callback to main class.

Testing

Existing tests should suffice.

nolan-veed commented 8 months ago

I'll fix the builds... wanted some early feedback.

gavv commented 8 months ago

Thanks for PR!

I'll do full review in upcoming days, but overall approach looks fine.

I think we can avoid node_release_callback: instead of iterating over elements in HashmapImpl::release_bucketarray using buckets, we can iterate them in template class destructor using front/back/nextof and release ownership in template class. Then implementation class will just deallocate buckets. After releasing ownership, and before deallocating buckets, buckets will contains dangling pointers to released elements, but that's not a problem because we're anyway in destructor.

nolan-veed commented 8 months ago

I think we can avoid node_release_callback: ...

I'll do that... it's better.

gavv commented 8 months ago

CI was failing because of problem in env-debian after recent rebuild of docker images. I've pushed a fix: https://github.com/roc-streaming/dockerfiles/commit/a2806619c7f55c00e2c0305b3a459e211348019c

gavv commented 8 months ago

Thanks, LGTM!

gavv commented 8 months ago

Small follow-up: de830bd2aef0433edbd760e08dc9c3f44f05228f