Skip to content

make inttypes.h optional#345

Closed
vszakats wants to merge 1 commit into
zlib-ng:devfrom
vszakats:inttypesopt
Closed

make inttypes.h optional#345
vszakats wants to merge 1 commit into
zlib-ng:devfrom
vszakats:inttypesopt

Conversation

@vszakats

@vszakats vszakats commented Nov 14, 2018

Copy link
Copy Markdown
Contributor

inttypes.h isn't available in certain environments, but instead of relying on CMake to create a fake local copy, let's handle the matter in the source code, and let it work with any build system. If HAVE_INTTYPES_H is defined, inttypes.h will be used, otherwise the required values will be defined locally.

`inttypes.h` isn't available in certain environments, but instead of
relying on CMake to create a fake local copy, let's handle the matter
in the source code, and let it work with any build system. If
`HAVE_INTTYPES_H` is defined, `inttypes.h` will be used, otherwise
the required values will be defined locally.
@akmistry

Copy link
Copy Markdown
Contributor

Can the condition be inverted (i.e. have NO_INTTYPES_H_ instead)? I think the default should be to assume a sane environment, and have the define available for those that need it.

@vszakats

vszakats commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

Speaking of toolchain features, to me the NO_INTTYPES_H method would look rather unusual, so I'd prefer to stick with the more familiar way. I also find it better to assume the least (or at least less) by default. Original minizip also didn't require inttypes.h. Finally, the implemented logic is exactly the same as used now, only shifted from CMake to the PP.

@nmoinvaz

Copy link
Copy Markdown
Member

I’m on vacation so I will take a look when I get back.

nmoinvaz added a commit that referenced this pull request Nov 20, 2018
nmoinvaz added a commit that referenced this pull request Nov 20, 2018
@nmoinvaz

nmoinvaz commented Nov 20, 2018

Copy link
Copy Markdown
Member

Sorry I did not merge your changes. I wasn't sure at the beginning I was going to do it so similar as your changes. Anyways it should be solved now. Thanks.

@nmoinvaz nmoinvaz closed this Nov 20, 2018
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.

3 participants