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

Add support for zlib compression with miniz (#163) #165

Merged

Conversation

FiniteReality
Copy link
Contributor

This allows users of luvi to use zlib compression without the zlib module, meaning that it is technically no longer necessary.

This has been tested on Debian buster (current testing) on a 64-bit AMD processor, where it appears to work correctly. I expect it to work correctly on all current targets.

I believe I have followed the style of code in my edits, though I will gladly fix any issues that are pointed out.

The API looks something like this, using the example of a deflator:

local miniz = require("miniz")
local deflator = miniz.new_deflator() -- optionally accepts the compression level to use
local deflated, err, partial = deflator:deflate("input text") -- optionally accepts a flush mode to use
-- if there was an error, deflate returns nil, the error that occured, as well as any partial data it managed to compress before an error occured.
print("Deflated data:")
print(deflated or partial)
if err then
    print("Error:")
    print(err)
end

src/lminiz.c Outdated
static int lmz_deflator_init(lua_State* L) {
int level = luaL_optint(L, 1, MZ_DEFAULT_COMPRESSION);
if (level < MZ_DEFAULT_COMPRESSION || level > MZ_BEST_COMPRESSION) {
luaL_error(L, "Compression level must be between -1 and 9");
Copy link
Member

Choose a reason for hiding this comment

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

Make sure everything uses 2 space indentations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, there goes KDevelop reindenting my code 😛

return 0;
}

static const char* flush_types[] = {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lua requires a string array for luaL_checkopt, which is where this is used.

Copy link
Member

@SinisterRectus SinisterRectus Oct 22, 2017

Choose a reason for hiding this comment

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

You probably want to use optint to stay consistent with inflate and deflate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those functions appear to use an integer because it's a bitfield, rather than an enumeration. Personally, I think using a string is more favourable here but I will gladly change it.

Copy link
Member

Choose a reason for hiding this comment

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

I guess either works.

@creationix
Copy link
Member

How much does this increase the binary size?

@FiniteReality
Copy link
Contributor Author

With the patch, binary size is increased by exactly 8 KiB on my machine.

@SinisterRectus
Copy link
Member

I know that zlib can optionally be used in luvi, but I was hoping that we could make the most out of miniz since it's included by default. I didn't realize that parts of it were disabled to keep the file size small.

@creationix
Copy link
Member

8kb is probably fine. Especially if it enables more apps to use the tiny version of luvi.

This allows users of luvi to use zlib compression without the zlib
module, meaning that it is technically no longer necessary.

This has been tested on Debian buster (current testing) on a 64-bit AMD
processor, where it appears to work correctly. I expect it to work
correctly on all current targets.
@FiniteReality FiniteReality force-pushed the feature/miniz-zlib-compression branch from 1379d0d to ec0c05c Compare October 24, 2017 13:54
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.

4 participants