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

feat: add bitmath inspector #128

Merged
merged 5 commits into from
Aug 9, 2023
Merged

feat: add bitmath inspector #128

merged 5 commits into from
Aug 9, 2023

Conversation

shellcromancer
Copy link
Contributor

@shellcromancer shellcromancer commented Aug 8, 2023

Description

This adds a new bitmath inspector similar to the strings inspector focused on comparison operations with numbers. The supported operations are and, or, xor, and not and compare the resulting value with 0.

Motivation and Context

Some datasets have field which are bitfield constants where the meaning is best unpacked with bitwise masking to check values and then inserting the meaning into a new field. I don't believe this is possible to check with the other inspectors today.

How Has This Been Tested?

New test cases have been added to cover the new existing operations and it solves for the target use case internally.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@shellcromancer shellcromancer marked this pull request as ready for review August 8, 2023 21:40
@shellcromancer shellcromancer requested a review from a team as a code owner August 8, 2023 21:40
Copy link
Collaborator

@jshlbrd jshlbrd left a comment

Choose a reason for hiding this comment

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

Overall looks good, added some comments for small improvements and requests for more unit testing.

Would this ever need to support floats?

func (c inspNumber) Inspect(ctx context.Context, capsule config.Capsule) (output bool, err error) {
var check int64
if c.Key == "" {
check = int64(binary.BigEndian.Uint64(capsule.Data()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if the data isn't a number? I think this needs a unit test, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched the parsing to be a little looser with size and format of interpretation by interpreting the byte slice input as a string, then parsing that string to a number using strconv.ParseInt and return an error when this condition is used without numeric input.

condition/number.go Outdated Show resolved Hide resolved
condition/number.go Outdated Show resolved Hide resolved
condition/number.go Outdated Show resolved Hide resolved
@shellcromancer
Copy link
Contributor Author

shellcromancer commented Aug 9, 2023

Overall looks good, added some comments for small improvements and requests for more unit testing.

Thanks for the quick review! :) Made some changes including the additional unit tests 👍

Would this ever need to support floats?

Today this inspector would handle floats in a loss-y way by truncating values into integers. There might be a use case for float handling considering that the JSON spec supports decimal notation (specifically arbitrary-precision decimal), but it's implementation should live separately b/c the key operation of bitwise-and masking isn't defined on floats in Go.

@jshlbrd
Copy link
Collaborator

jshlbrd commented Aug 9, 2023

Today this inspector would handle floats in a loss-y way by truncating values into integers. There might be a use case for float handling considering that the JSON spec supports decimal notation (specifically arbitrary-precision decimal), but it's implementation should live separately b/c the key operation of bitwise-and masking isn't defined on floats in Go.

It sounds like two different inspectors (number and bitwise) or this one inspector handles the value differently depending on the type.

@shellcromancer
Copy link
Contributor Author

It sounds like two different inspectors (number and bitwise) or this one inspector handles the value differently depending on the type.

With that in mind I've focused this on doing bitmath and then comparing for a non-zero result instead and removed non-bitwise types 👍

Copy link
Collaborator

@jshlbrd jshlbrd left a comment

Choose a reason for hiding this comment

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

Approved and changes look good! I added some comments that should be addressed before merging. Could you also change the PR name to be more reflective of the new direction?

condition/bitmath.go Outdated Show resolved Hide resolved
condition/bitmath.go Show resolved Hide resolved
@shellcromancer shellcromancer changed the title feat: add number inspector feat: add bitmath inspector Aug 9, 2023
@shellcromancer shellcromancer merged commit 4721ffa into main Aug 9, 2023
6 checks passed
@shellcromancer shellcromancer deleted the shellcromancer/num-insp branch August 9, 2023 22:33
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