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

inttypes and stdint cleanup #30

Merged
merged 2 commits into from
Oct 29, 2021
Merged

Conversation

carenas
Copy link
Contributor

@carenas carenas commented Oct 24, 2021

While testing the proposed changes in #28, noticed that the Windows build was broken for anything older than MSVC 2013 (which ironically would make the fallback unnecessary, at least in Windows).

Interestingly, even if the change that triggered this issues would be reverted, that wouldn't fix the builds with MSVC 2010 and MSVC 2012 that provide stdint.h (but not inttypes.h) as that is not enough to build with JIT enabled, which remains broken even if the suggested stdint.h replacement header would be used instead.

MSVC 2012 is still supported, but I suspect the default SDK it was using (which I got from appveyor) is probably set to an OS that will be soon EOL as well, which might explain why nobody complained, hence the next best thing would be to complete the cleanup of this code which is done in the two following patches and as a side effect make the configuration slightly faster.

PS: the generated "html" version of NON_AUTOTOOLS_BUILD wasn't updated

Since 19c50b9 (Unconditionally use inttypes.h instead of trying for
stdint.h (simplification) and remove the now unnecessary inclusion in
pcre2_internal.h., 2018-11-14), stdint.h is no longer used.

Remove checks for it in autotools and CMake and document better the
expected build failures for systems that might have stdint.h (C99)
and not inttypes.h (from POSIX), like old Windows.
CMake checks for standard headers are not meant to be used for hard
dependencies, so will prevent a possible fallback to work.

Alternatively, the header could be checked to make the configuration
fail instead of breaking the build, but that was punted, as it was
missing anyway from autotools.
@PhilipHazel
Copy link
Collaborator

Thanks for this and your other updates. I want to acknowledge that I've seen them and will act in due course, but just at the moment I'm busy on an entirely different project so I'm leaving them for a week or two as there is no urgency.

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.

2 participants