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

#663: Fixed bitcount operations #699

Conversation

VedWhat
Copy link
Contributor

@VedWhat VedWhat commented Sep 23, 2024

This PR attempts to close #663 however I realised rewrite would be a better option. This passes all tests under tcl (only for bitcount) and all integration tests. While this also fixes other issues, I don't want to overstep.

I've added comments wherever necessary but can elaborate wherever necessary

What has changed?

  • Added better argument validation
  • Changed counting in a loop to count using a bitmask
  • Added better handling of start and end indices.

@lucifercr07 as mentioned, this does fix the other bitcount issues, lmk what you think. I believe this is a better way to fix the errors (fixing a single test would've made it messier if you get what I mean)

Happy to change the approach just to fix the single test.

Copy link
Contributor

@lucifercr07 lucifercr07 left a comment

Choose a reason for hiding this comment

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

LGTM, it's more cleaner than prior implementation.
@JyotinderSingh shall we merge this?

@VedWhat on other note what are your thoughts on using go inbuilt function i.e. OnesCount8 rather than the popcount implementation?

Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment

Choose a reason for hiding this comment

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

LGTM. Few minor comments.

internal/eval/eval.go Outdated Show resolved Hide resolved
internal/eval/eval.go Outdated Show resolved Hide resolved
@VedWhat VedWhat force-pushed the inconsistent_bitcount_fuzzing_without_start_end branch from 257ba8b to 9415d55 Compare September 24, 2024 12:12
@VedWhat
Copy link
Contributor Author

VedWhat commented Sep 24, 2024

LGTM, it's more cleaner than prior implementation. @JyotinderSingh shall we merge this?

@VedWhat on other note what are your thoughts on using go inbuilt function i.e. OnesCount8 rather than the popcount implementation?

Nice, makes sense to use a lookup instead of any operations. Made the change!

@JyotinderSingh
Copy link
Collaborator

LGTM, it's more cleaner than prior implementation. @JyotinderSingh shall we merge this?

@VedWhat on other note what are your thoughts on using go inbuilt function i.e. OnesCount8 rather than the popcount implementation?

Nice, makes sense to use a lookup instead of any operations. Made the change!

thanks, could you take a look at the linter issues too?

@VedWhat
Copy link
Contributor Author

VedWhat commented Sep 24, 2024

LGTM, it's more cleaner than prior implementation. @JyotinderSingh shall we merge this?

@VedWhat on other note what are your thoughts on using go inbuilt function i.e. OnesCount8 rather than the popcount implementation?

Nice, makes sense to use a lookup instead of any operations. Made the change!

thanks, could you take a look at the linter issues too?

right, was pushing with a no-verify due to an issue. already pushed the change.

@JyotinderSingh JyotinderSingh changed the title Fixed bitcount operations #663: Fixed bitcount operations Sep 24, 2024
@JyotinderSingh JyotinderSingh merged commit 2d98469 into DiceDB:master Sep 24, 2024
2 checks passed
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.

Inconsistent BITCOUNT: BITCOUNT fuzzing without start/end
3 participants