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.

Sign up for GitHub

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

Fail if duplicate keys are present #177

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fail if duplicate keys are present #177

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7a15b7
Fail if duplicate keys are present
Ahajha Oct 1, 2024
52bb2dd
Incorporate review feedback
Ahajha Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/frozen/bits/pmh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ pmh_buckets<M> constexpr make_pmh_buckets(const carray<Item, N> & items,
bool rejected = false;
for (std::size_t i = 0; i < items.size(); ++i) {
auto & bucket = result.buckets[hash(key(items[i]), static_cast<std::size_t>(result.seed)) % M];
for (const auto item_index : bucket) {
if (key(items[item_index]) == key(items[i])) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This should use the KeyEqual parameter.
  • This would probably be better if key(items[i]) were saved in a temporary variable.
  • move this loop to an helper function?
  • Move the check under ifndef NDEBUG ?
  • Instead of calling (exit), what about
+      extern void check(const char[]);
+      check("Duplicate keys present, check your input data");

Copy link
Author

@Ahajha Ahajha Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all of these except maybe the ifndef NDEBUG comment. For my use case, I'm writing a library where the keys will end up being supplied by the user. I think it would be better for their experience if they always saw a helpful message, even in release mode. Perhaps we could add a macro that would force the check to happen, but by default it's only in debug mode? So something like #if !defined NDEBUG || defined FROZEN_LETITGO_ENABLE_DUPLICATE_KEY_ASSERTIONS? (we could also default that variable to be defined with NDEBUG, and the user can manually enable it, which simplifies the check)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if adding another preprocessor directive, and also since hopefully the old error message should go away in debug mode, should I update the README to mention the new macro?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some running commentary: Extracting that check to a separate function might be more trouble than it's worth, due to the inputs it ends up needing to be heavily templated and type annotated. I'll try to get it working just to show what it would look like.

I'll need to pass KeyEqual down the stack quite a bit - though this is for correctness so that's fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've incorporated everything except the NDEBUG comment, once we reach a consensus I'll add that in.

(void)"Duplicate keys present, check your input data"; exit(1);
}
}
if (bucket.size() >= result_t::bucket_max) {
rejected = true;
break;
Expand Down