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

refactor interface for static state allocation #1109

Merged
merged 2 commits into from
Jul 12, 2022
Merged

refactor interface for static state allocation #1109

merged 2 commits into from
Jul 12, 2022

Conversation

Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Jul 12, 2022

#1104 fixed an issue impacting the size of states on OS400 specifically. This is a relatively niche scenario, when statically allocating on stack or as part of another structure (which is not recommended, though allowed).

To do that, @jonrumsey fixed the manual size of LZ4_STREAMSIZE and other equivalent constants, to fit the scenario of OS400, where pointers are 128-bit large.
And while the fix is correct, it should not have been necessary.

The interface design requires employing objects such as LZ4_stream_t to allocate statically, and never the constants nor internal definitions. LZ4_STREAMSIZE only exists first due to historical reasons, but also as a way to protect against mismatched library versions. In essence, it guarantees that the state preserves a "minimal size", in case it would evolve or shrink in the future, so that it remains compatible with older library versions.

The problem was that this "minimal state size", which is generally >= to the actual state size, ends up being lower that actual state size in the case OS400.
Still, this shouldn't be a problem, because in this case, the state occupies its actual size.

The problem came from the fact that LZ4_STREAMSIZE was used in other parts of the source code, and notably within static assert, resulting in compilation errors. And this wasn't a correct usage of this constant.

This PR fix the underlying issue, by making sure that all code now use sizeof(LZ4_stream_t), and no longer LZ4_STREAMSIZE, which by the way has been renamed LZ4_STREAM_MINSIZE, to better reflect its role.

Such update is also easier to validate thanks to previously merged ABI compatibility test.

@Cyan4973 Cyan4973 changed the title refactor API for static allocation refactor interface for static state allocation Jul 12, 2022
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.

None yet

1 participant