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

Make bitmap of arbitrary size #211

Merged
merged 5 commits into from
Feb 24, 2023
Merged

Make bitmap of arbitrary size #211

merged 5 commits into from
Feb 24, 2023

Conversation

alovak
Copy link
Contributor

@alovak alovak commented Jan 31, 2023

What

  • adding DisableAutoExpand (bool) to the field spec. Used by the field.Bitmap
  • if DisableAutoExpand is set to true, then field.Bitmap will be just a fixed bitmap
  • remove min and max bitmap size - now you can create bitmaps with arbitrary length (just set DisableAutoExpand to true)

Example of how to create fixed bitmap:

	bitmap := NewBitmap(&Spec{
		Length:            2, // 2 bytes - 16 bits
		Description:       "Bitmap",
		Enc:               encoding.BytesToASCIIHex,
		Pref:              prefix.Hex.Fixed,
		DisableAutoExpand: true,
	})

	// when setting bits inside of bitmap range
	bitmap.Set(1)
	bitmap.Set(16)
        bitmap.Set(17) // is ignored as 17 is out of range

Why

P.S. Note, for most card brands, the default single bitmap size is 8 bytes (64 bits). Because of the previous implementation, the Length parameter was ignored and instead underlying storage for the 3 (max allowed) bitmaps was used. For all operations, it calculated the number of bitmaps based on the bits set. So, if for auto-expanding bitmaps you use 16 or something else, then bitmap will be expanded by 16 bytes for new bitmaps. That's not what you want. Just change from 16 to 8.

@adamdecaf
Copy link
Member

It sounds like AutoExpand: false is the current behavior so that should be the default. Changing this into an auto-expanding bitmap (by default) will break existing code.

@mfdeveloper508
Copy link
Contributor

LGTM
the approach is good

my advice is

  • remove DisableAutoExpand option
  • AutoExpand is active if there isn't any length setting
  • AutoExpand is inactive if there is any length setting

@alovak
Copy link
Contributor Author

alovak commented Jan 31, 2023

@adamdecaf our current behaviour is AutoExpand: true that's why I had to introduce option that is false by default (negative naming).

@alovak
Copy link
Contributor Author

alovak commented Jan 31, 2023

@mfdeveloper508 thanks for the suggestions

I tried to not break anything. Now integration will be broken only if Length is different from 8. If it's not, then new version of the package will not require any changes from you.

I like your proposal, but I'm afraid as it's a not backward compatible change, I would vote for DisableAutoExpand.
,

remove DisableAutoExpand option
AutoExpand is active if there isn't any length setting
AutoExpand is inactive if there is any length setting
^^ this will force people to make updates to their systems as the default use case is auto expand and I believe most of the time Lenght is specified.

I'm also not aware of cases with auto-expanding bitmaps of length different than 8. But maybe there are such cases?

field/bitmap.go Outdated Show resolved Hide resolved
@adamdecaf
Copy link
Member

Thanks for making it backwards compatible.

@alovak alovak merged commit 0607a57 into master Feb 24, 2023
@alovak alovak deleted the make-bitmap-of-arbitrary-size branch February 24, 2023 15:45
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.

3 participants