Closed pedromfcarvalho closed 2 years ago
since I still have a bunch of issue to resolve.
What kind of issues are you having?
What kind of issues are you having?
I had to change the types of some functions to be able to compile this. For instance, nx_crc.c didn't include
In the new api files (e.g. nx_zlib_api_crc32.c), I included
Function crc32 used to take a uint64_t for the len parameter, but the zlib version takes an int, so I changed this (in nx_zlib_api_crc32.c).
Function crc32_combine took an uint64_t for the len2 parameter, and I had to change it to z_off_t to match zlib.
Function crc32_combine64 also took an uint64_t for len2, and I changed it to z_off64_t.
Functions adler32 and adler32_z take a const char for buf, which I had to change to const unsigned char .
I'm not sure if changing these type is ok. In the case of function crc32 calls crc32_ppc which itself takes an unsigned for len (i.e. unsigned int), but if we might change it in the future for a call to nx_crc32, which itself takes a uint64_t. That being said, we might have to fix nx_crc32 to take a regular int to match what zlib does.
Like our nx_ declarations, I also didn't add any of the zlib qualifiers (ZEXTERN, ZEXPORT, etc.)
Because some of the nx_ functions are not exported in "nx_zlib.h", I had to re-declare them. This could be risky since we might decide to fix some of the paramater types for these functions and put them in "nx_zlib.h", so we can't forget to remove the re-declarations. This wasn't an issue before because the api functions were defined in the same files as the underlying nx_functions were defined.
I declared some of the Makefile variables in config.mk, such as PACKAGENAME and STATICLIB_NO_API, because I needed them both in lib/Makefile and test/Makefile. However, they are not something that the user should change. Should this go into a different .mk file that can be included by other makefiles?
* **Type changes in functions**
I had to change the types of some functions to be able to compile this. For instance, nx_crc.c didn't include
, so there was no mismatch at detected before at compile time. In the new api files (e.g. nx_zlib_api_crc32.c), I included
to get some of the type definitions, and "nxzlib.h" to get the declarations for the nx functions. The nx_crc.c and nx_adler32.c files didn't need this because it re-declared the needed types (e.g. uLong), but I don't think this is an ideal solution.
I believe we should go towards not needing to include zlib.h anywhere, I agree that redeclaring the zlib types isn't a good solution, I prefer to use the original C types or standards like from stdlib or stdint.
* Function crc32 used to take a uint64_t for the len parameter, but the zlib version takes an int, so I changed this (in nx_zlib_api_crc32.c). * Function crc32_combine took an uint64_t for the len2 parameter, and I had to change it to z_off_t to match zlib. * Function crc32_combine64 also took an uint64_t for len2, and I changed it to z_off64_t. * Functions adler32 and adler32_z take a const char * for buf, which I had to change to const unsigned char *.
I'm not sure if changing these type is ok. In the case of function crc32 calls crc32_ppc which itself takes an unsigned for len (i.e. unsigned int), but if we might change it in the future for a call to nx_crc32, which itself takes a uint64_t. That being said, we might have to fix nx_crc32 to take a regular int to match what zlib does.
It's ok to change these types but you have to review these mismatches to be sure to pick the right ones, I don't think zlib should be our reference on this, we should have compatible types because we are implementing the zlib API but because it uses custom types we shouldn't use these blindly. Also, you are essentially doing https://github.com/libnxz/power-gzip/issues/33, so you should assign it to yourself to avoid other people working on it at the same time. :)
Like our nx_ declarations, I also didn't add any of the zlib qualifiers (ZEXTERN, ZEXPORT, etc.)
I guess we have all qualifiers we need, you could review it but we should use only C qualifiers like extern and export.
* **Re-declarations**
Because some of the nx_ functions are not exported in "nx_zlib.h", I had to re-declare them. This could be risky since we might decide to fix some of the paramater types for these functions and put them in "nx_zlib.h", so we can't forget to remove the re-declarations. This wasn't an issue before because the api functions were defined in the same files as the underlying nx_functions were defined.
Can't you use libnxz.h? Is there any function missing there?
* **config.mk**
I declared some of the Makefile variables in config.mk, such as PACKAGENAME and STATICLIB_NO_API, because I needed them both in lib/Makefile and test/Makefile. However, they are not something that the user should change. Should this go into a different .mk file that can be included by other makefiles?
I don't see a problem on having them in configs.mk, an user could change if they are in the Makefiles as well.
This code doesn't apply to the current code anymore, and with the sw_*
functions added we have a simpler way to implement this.
Closing.
This is a draft PR for fixing tests that are meant to compare zlib and libnxz output, since the currently compare libnxz output with itself when the zlib interface symbols are built.
This is a draft PR since I still have a bunch of issue to resolve.