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

Addition of paletteSize, while removing private field of Palette' #184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Jan 5, 2020

From what I gather _paletteSize is an unnecessary field in Palette', since it can be deduced from the size of _paletteData and the type of pixel, which is exactly the way that the newly added paletteSize is implemented. Couple benefits that come from this change:

  • Palette' is now a newtype around Vector, hence slight reduction to memory usage and overall improvement to performance
  • Safety improvement: Exposing private _paletteSize that is coupled to the actual data allows construction of incorrect Palette', which opens a possibility of a segfault when using images converted from such palettes by palettedAsImage

@Twinside
Copy link
Owner

At this point, while this is a worthwhile enhancement, I wouldn't break compatibility for such "low" issue, has to keep it in mind in case of bigger breaking change

@lehins
Copy link
Contributor Author

lehins commented Feb 24, 2020

@Twinside I agree, that bundling this together with a bigger breaking change is a good idea.

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