Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the insert operation #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdashti
Copy link

@mdashti mdashti commented Jan 17, 2018

This pull-request adds an insert operation (which simply fails when a mapping already exists). This operation simplifies the usage and avoids unnecessary locking on the user side and it specially helps when the value is dynamically allocated (as opposed to the solutions provided in http://preshing.com/20160201/new-concurrent-hash-maps-for-cpp).

@preshing
Copy link
Owner

Hi,

Thanks for your work! I think I see where your point.

Just a few problems with the pull request:

  • There are no tests. A new feature like this warrants a .cpp file for a new test in MapCorrectnessTests Ideally the test would simulate many threads racing to insert dynamically allocates and validate that there are no leaks or double frees.
  • For this feature I'd prefer the names tryInsert and tryInsertValue instead of insert and putValueIfNotExists. (I realize std::map defines insert but I'm not modeling after std::map, and "try" is a little more descriptive.)
  • The TURF_TRACE macros should be prefixed with [Mutator::tryInsertValue] as well (not [Mutator::exchangeValue]) and the trace indices should be cleaned up with turf/extra/TidySource.py. We should also make sure the tests exercise every codepath when run. I realize the trace system hasn't been documented anywhere; I could provide more info if you want.
  • Would be good to factor out these lines to a common function.
  • Feature not implemented in the Linear or Leapfrog maps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants