docs: document the need for a 64-bit type and stdint.h#20384
docs: document the need for a 64-bit type and stdint.h#20384
Conversation
|
There is still a |
docs/INTERNALS.md
Outdated
| machines. Most of libcurl assumes more or less POSIX compliance but that is | ||
| not a requirement. | ||
| not a requirement. The compiler must support a 64-bit integer type and supply | ||
| the stdint.h header file (which was not strictly required before C99). |
There was a problem hiding this comment.
We check for the stdint.h file and can manage without it. I believe the OS400 build right now even needs it to be set not present.
There was a problem hiding this comment.
Where are the fixed-size integer types defined then if not stdint.h? inttypes.h? So, does at least one of those files need to exist so those types are defined?
There was a problem hiding this comment.
inttypes.h isn't included. Existing pre-fills set HAVE_STDINT_H to 1. Patrick commented recently, that OS400 does provides this header (albeit without the optional uintptr_t). I guess config-os400.h does not set it because if set, formdata.h uses uintptr_h (since fe7703a), also curl_setup.h (since f4e2395). uintptr_t is also used in Rustls- and libbacktrace-conditional code.
curl required C99 types for H2, H3 and wolfSSL for a long time, without checking HAVE_STDINT_H, and without reports about it.
With wider use of 64-bit and non-64-bit C99 types, I'd guess stdint.h is a hard requirement now, and documenting it is useful IMO. Also the checks could maybe dropped to clarify. For uintptr_t we can use a fallback for OS400 (and other platforms, but so far there is no more we know about).
There was a problem hiding this comment.
I've reworded the text to make it less ambiguous and to mention inttypes.h as an option.
There was a problem hiding this comment.
I wonder if we shall mention inttypes.h? Both are C99, and we already include stdint.h.
To support both, we'd need to do all the configure work, and in most cases include both.
This looks like a waste of cycles for every build.
(MSVC is one outlier that introduced stdint.h first (VS2010), then inttypes.h in VS2013.
That's one reason curl uses stdint.h.
These are requirements above and above C89. Closes #20384
59bfe48 to
e6c80e4
Compare
|
Analysis of PR #20384 at e6c80e41: Test 310 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 12 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (5, to be exact) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
These are requirements above and above C89. Closes #20384
e6c80e4 to
3c5aea9
Compare
These are requirements above and above C89. Closes #20384
3c5aea9 to
81dd66f
Compare
|
My memory is that inttypes.h was used to hold typedefs on various systems
before C99, and C99 standardized that type definitions go in stdint.h and
printf specifiers go in inttypes.h. Back in 2001 I wrote some autoconf macros
to detect the fixed-size types in both inttypes.h and stdint.h but prefer the
stdint.h ones because they were standardized. I assumed that's what was
happening in curl, too, but it seems like inttypes.h there is only used for
Windows.
In looking at the includes more now, it looks like the fixed-size types are
expected to be found in stdint.h now. Can we drop the entire HAVE_STDINT_H
dance now and expect it to be there? I actually thought that was what was being
done. I'll create a PR and see what CI thinks about that.
|
curl now expects C99-style fixed-length types to be defined, and the standard location for that is stdint.h. There is no need to check for a header file that must exist. Caveats: - Some systems predating C99 defined the necessary types in inttypes.h, but this PR doesn't affect those. - Apparently, OS400 doesn't define uintptr_t but does have stdint.h, so this PR will probably break it. The uintptr_t dependencies should probably be guarded by a new HAVE_UINTPTR_T macro (or maybe MISSING_UINTPTR_T) to use alternate code there. Ref: #20384
It seems safe to drop, yes. I also prepared a patch a few days ago. There is no more |
|
There is no more inttypes.h in curl though. (but there is in libssh2)
It's still included in two Windows cases in curl/system.h, but it looks like that's to get the printf specifiers so I guess it's OK.
|
There was a problem hiding this comment.
Pull request overview
This PR documents compiler requirements for curl/libcurl that extend beyond the C89 standard, specifically the need for 64-bit integer type support and C99-style fixed-width integer types from stdint.h or inttypes.h.
Changes:
- Updated INTERNALS.md to document the requirement for 64-bit integer types and stdint.h/inttypes.h headers
- Added related terms to the spell-checking dictionary
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/INTERNALS.md | Added documentation clarifying that while curl targets C89 compilers, it requires support for 64-bit integers and C99-style fixed-width types from stdint.h or inttypes.h |
| .github/scripts/pyspelling.words | Added "inttypes", "stdint", and "uint" to the spell-checking dictionary to support the documentation changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
curl requires `stdint.h` from C99, and no longer builds without it since v8.18.0 (after dropping VS2008 support). Assume it's available, drop feature checks. Also: - drop duplicate `stdint.h` includes. - introduce internal `HAVE_UINTPTR_T`, enabled by default. - OS400: disable `HAVE_UINTPTR_T`. - build: keep cmake pre-fill and `cmp-config.pl` exception because cmake and autotools both detect `stdint.h` implicitly. Co-authored-by: Dan Fandrich Ref: #20405 Ref: #20384 Follow-up to 2e1a045 #17931 Closes #20406
|
I'd still reduce the text to |
|
Yes, thanks for making that part more consistent. |
These are requirements above and above C89.
Closes #20384