search

vgmtrans / vgmtrans

VGMTrans - a tool to convert proprietary, sequenced videogame music to industry-standard formats
812 stars 78 forks source link

Clang warning fixes #475

Closed TheComputerGuy96 closed 1 month ago

TheComputerGuy96 commented 1 month ago

This should be enough to finally enable -Werror in non-MSVC cases (MSVC uses a /WX flag for that instead) :frog:

For the zlib issue I could've just fixed the problematic prototype part but updating zlib probably brings some security fixes too so I think that's a better option

mikelow commented 1 month ago

Did you do any testing? You made a number of logic changes in format code.

mikelow commented 1 month ago

@TheComputerGuy96 from where did you get this version of zlib? @sykhro should we make zlib a git submodule given that we're doing that for spdlog?

sykhro commented 1 month ago

@TheComputerGuy96 from where did you get this version of zlib? @sykhro should we make zlib a git submodule given that we're doing that for spdlog?

Yes, at that point we could even switch to https://github.com/zlib-ng/zlib-ng/

mikelow commented 1 month ago

@TheComputerGuy96 I'm happy to make a new branch that brings in your commits sans the vendored zlib, fixes the issues I mentioned, and introduces zlib-ng via git submodule.

TheComputerGuy96 commented 1 month ago

from where did you get this version of zlib?

I unpacked the zlib 1.3.1 tarball (specifically https://github.com/madler/zlib/releases/download/v1.3.1/zlib-1.3.1.tar.gz) and started removing files from it to reduce the space of the module and modified CMakeLists,txt to remove the shared library stuff and build the minizip library together too

At that point we could even switch to https://github.com/zlib-ng/zlib-ng/

I'm fine with this because Arch has packaged both zlib-ng and minizip-ng (so a submodule won't be needed there)

TheComputerGuy96 commented 1 month ago

Did you do any testing? You made a number of logic changes in format code.

I would have to get music of multiple different formats to test this PR properly

mikelow commented 1 month ago

Closing this now that it has been integrated into #477 and merged.

I am working on replacing zlib/minizip with zlib-ng/minizip-ng and including them as git submodules. I will put up that PR when it is ready.