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

Use ByteString instead of std::vector in the BloomFilter #11038

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

dconeybe
Copy link
Contributor

No description provided.

@dconeybe dconeybe self-assigned this Mar 29, 2023
@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@dconeybe dconeybe changed the base branch from master to mila/BloomFilter March 29, 2023 16:54
@dconeybe dconeybe marked this pull request as ready for review March 29, 2023 19:52
@dconeybe dconeybe requested a review from milaGGL March 29, 2023 19:52
const std::vector<uint8_t>& bitmap1 = lhs.bitmap();
const std::vector<uint8_t>& bitmap2 = rhs.bitmap();
const auto byte_count = static_cast<int32_t>(bitmap1.size());
const auto byte_count = static_cast<int32_t>(lhs.bitmap().size());
Copy link
Contributor

@milaGGL milaGGL Mar 30, 2023

Choose a reason for hiding this comment

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

I think i asked the same question before, but why auto? since we know its type will be int32_t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lint suggests (requires?) using auto when the type is explicitly mentioned on the right-hand side of the equal sign. By mentioning it twice it actually opens up the opportunity for an unintended implicit narrowing conversion if there is a typo on one side of the equal sign.

Copy link
Contributor

@milaGGL milaGGL left a comment

Choose a reason for hiding this comment

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

LGTM

@dconeybe dconeybe merged commit 5b83bd4 into mila/BloomFilter Mar 30, 2023
@dconeybe dconeybe deleted the dconeybe/BloomFilterByteString branch March 30, 2023 00:45
@firebase firebase locked and limited conversation to collaborators Apr 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants