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

Better C integer definitions #6645

Merged
merged 10 commits into from Jun 24, 2023
Merged

Better C integer definitions #6645

merged 10 commits into from Jun 24, 2023

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Oct 7, 2022

  1. Fix INT64 definition where it was checking the size of long long but then defining it as long.
  2. Add compile warning (not an error) if INT64 wasn't defined.
  3. Add labels to some preprocessor if/else/endifs to make it more clear which ones go together.
  4. Add comment explaining why we're using #define instead of typedef.
  5. Check for C99 support and use the values in stdint.h if possible.
  6. Move the INT8 definition to the top, so that they're all in order by size.
  7. Change the INT16 definition failure to an error instead of silently using the wrong value. There was a comment saying things mostly worked anyways, but I'm skeptical of that, and also we were already failing if INT32 couldn't be defined, so I doubt it would have gotten past that point anyways.

src/libImaging/ImPlatform.h Outdated Show resolved Hide resolved
src/libImaging/ImPlatform.h Outdated Show resolved Hide resolved
src/libImaging/ImPlatform.h Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented May 19, 2023

What makes these definitions better?

Do we need to support both C99 and pre-C99?

What is the minimum needed for CPython itself, or other libraries such as NumPy?

@radarhere
Copy link
Member

The oldest version of GCC I could find that we are using is 4.8.5 - https://github.com/python-pillow/docker-images/actions/runs/4813262439/jobs/8569570838#step:6:99

https://gcc.gnu.org/onlinedocs/gcc-4.8.5/cpp.pdf

The value 199409L signifies the 1989 C standard as amended in 1994, which is the current default; the value 199901L signifies the 1999 revision of the C standard. Support for the 1999 revision is not yet complete.

@hugovk
Copy link
Member

hugovk commented May 19, 2023

What is the minimum needed for CPython itself [...]?

PEP 7 says:

  • Python 3.11 and newer versions use C11 without optional features. The public C API should be compatible with C++.
  • Python 3.6 to 3.10 use C89 with several select C99 features:
    • Standard integer types in <stdint.h> and <inttypes.h>. We require the fixed width integer types.
    • static inline functions
    • designated initializers (especially nice for type declarations)
    • intermingled declarations
    • booleans
    • C++-style line comments

@hugovk
Copy link
Member

hugovk commented May 19, 2023

What is the minimum needed for [...] other libraries such as NumPy?

https://numpy.org/doc/stable/user/building.html says:

Much of NumPy is written in C. You will need a C compiler that complies with the C99 standard.

@Yay295
Copy link
Contributor Author

Yay295 commented May 19, 2023

<stdint.h> is better because it's standardized, so we have a guarantee about what exists.

We previously discussed the minimum C version here: #6516
The oldest we found was GCC on CentOS 7 which uses gnu90 by default.

If we could guarantee that <stdint.h> exists we could simplify this file a lot, but it doesn't seem like we can do that yet.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@hugovk
Copy link
Member

hugovk commented May 19, 2023

Thanks. And CentOS 7 is EOL on 30 Jun 2024.

We could consider requiring a newer C for Pillow 10.4, due out the next day on 1 Jul 2024, but I'd be more comfortable rolling it into the next quarterly release seeing as it's a major bump (11.0, 15 Oct 2024, supporting Python 3.9+).

And we may want to keep support for older C versions longer, but let's re-evaluate in a year or so.

@hugovk
Copy link
Member

hugovk commented May 19, 2023

@nulano How does this look from the Windows perspective?

@nulano
Copy link
Contributor

nulano commented May 22, 2023

I don't see any Windows changes in this PR.
The long long fix looks like it would be Windows-specific, but it is in a non-Windows block, which is presumably why it hasn't been noticed until now.

If we could guarantee that <stdint.h> exists we could simplify this file a lot, but it doesn't seem like we can do that yet.

I think this also applies to Windows. I'm not sure if it is guaranteed to exist on Windows, but if it might not be present on other platforms, I see no reason to worry about it yet.

@radarhere radarhere merged commit 0d54cf5 into python-pillow:main Jun 24, 2023
61 checks passed
@Yay295 Yay295 deleted the int_def branch June 24, 2023 04:21
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.

None yet

5 participants