Skip to content

Conversation

@bagder
Copy link
Member

@bagder bagder commented Jan 7, 2026

In particular, it turns 'unsigned long' into 'uint32_t' since the code needs to build and run just as fine on Windows which has 32 bit longs, so we know the code works with 32 bit field versions.

This makes Curl_easy 56 bytes smaller on my 64 bit Linux (maximized build).

@bagder bagder requested a review from Copilot January 7, 2026 14:49
@bagder bagder marked this pull request as ready for review January 7, 2026 14:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR converts various integer types in lib/urldata.h and related files from platform-dependent types (unsigned long, unsigned int, unsigned char, unsigned short) to fixed-width integer types (uint32_t, uint16_t, uint8_t). This improves portability across platforms, particularly Windows where long is 32 bits, and reduces the size of the Curl_easy struct by 56 bytes on 64-bit Linux.

  • Replaced platform-dependent unsigned types with fixed-width integer types
  • Added explicit casts in setopt.c to handle type conversions
  • Updated function signatures in http.c for consistency

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
lib/urldata.h Converted struct member types from unsigned long/int/short/char to fixed-width types (uint32_t, uint16_t, uint8_t) across multiple structs including ssl_primary_config, Curl_handler, connectdata, PureInfo, auth, UrlState, UserDefined, and Curl_easy
lib/setopt.c Added explicit casts to uint32_t when assigning authentication values to proxyauth and httpauth fields
lib/http.c Updated function parameter types from unsigned long * to uint32_t * in authentication-related functions (auth_spnego, auth_ntlm, auth_digest, auth_basic, auth_bearer)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@testclutch
Copy link

Analysis of PR #20209 at f5499a4c:

Test 3035 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@vszakats
Copy link
Member

vszakats commented Jan 7, 2026

With a rebase it'll now run on DJGPP for an extra test.

@bagder bagder force-pushed the bagder/urldata-uint branch from f5499a4 to 5da0147 Compare January 7, 2026 21:07
@bagder
Copy link
Member Author

bagder commented Jan 7, 2026

With a rebase it'll now run on DJGPP for an extra test.

I think I'm going to file a complaint on this. This is an uint32_t, why is %u wrong here? I don't think this is a code problem, but a djgpp problem.

 3855 |       failf(data, "could not allocate xfer_buf of %u bytes",
      |                                                   ~^
      |                                                    |
      |                                                    unsigned int
      |                                                   %lu
 3856 |             data->set.buffer_size);
      |             ~~~~~~~~~~~~~~~~~~~~~                   
      |                      |
      |                      uint32_t {aka long unsigned int}

@bagder
Copy link
Member Author

bagder commented Jan 7, 2026

I think I'm going to file a complaint on this. This is an uint32_t, why is %u wrong here? I don't think this is a code problem, but a djgpp problem.

With this I mean that I don't think we should fix this with using a define that changes %u to something else on djgpp builds. We should make djgpp accept %u; perhaps by changing its uint32_t type to an unsigned int - or to disable the warning when using that compiler.

I think changing %u to a define reduces readability for an edge case crazy compiler.

@vszakats
Copy link
Member

vszakats commented Jan 7, 2026

With a rebase it'll now run on DJGPP for an extra test.

I think I'm going to file a complaint on this. This is an uint32_t, why is %u wrong here? I don't think this is a code problem, but a djgpp problem.

 3855 |       failf(data, "could not allocate xfer_buf of %u bytes",
      |                                                   ~^
      |                                                    |
      |                                                    unsigned int
      |                                                   %lu
 3856 |             data->set.buffer_size);
      |             ~~~~~~~~~~~~~~~~~~~~~                   
      |                      |
      |                      uint32_t {aka long unsigned int}

I don't know. I also found it odd that it's only DJGPP complaining.

My understanding is that for uint32_t one is supposed to use
a matching PRI*32 mask, which may map to u or ul depending
on platform. I committed the necessary header and fallbacks earlier
today, to allow using them.

It didn't occur to me this is a DJGPP bug. It also picked other (non-printf)
cases in tests where unsigned int was not updated to uint32_t.
Also not caught in CI, only by DJGPP:
https://github.com/curl/curl/actions/runs/20786707385/job/59698125361

Seems legit to me, after all, it's not guaranteed that unsigned int is
the exact same type as uint32_t. (I fixed in 8881a52.)

@bagder
Copy link
Member Author

bagder commented Jan 7, 2026

My understanding is that for uint32_t one is supposed to use a matching PRI*32 mask, which may map to u or ul depending on platform.

We use our own printf() code. We know it works with 32 bit fields for %u. Adding warnings for this is just annoying for us. Forcing us to use defines just because djgpp insists on this I think is the wrong path.

@bagder
Copy link
Member Author

bagder commented Jan 7, 2026

I think a much better take is to disable this warning when djgpp is used.

@vszakats
Copy link
Member

vszakats commented Jan 7, 2026

My understanding is that for uint32_t one is supposed to use a matching PRI*32 mask, which may map to u or ul depending on platform.

We use our own printf() code. We know it works with 32 bit fields for %u. Adding warnings for this is just annoying for us. Forcing us to use defines just because djgpp insists on this I think is the wrong path.

It's kind of impossible to predict these, sorry. I spent my entire day fixing this,
after some AI yelled at me a (valid) MS-DOS regression, so that I had to spin
up DJGPP locally, just to find a bunch of pre-existing issues. Then more.

Super annoying indeed.

Though I think the warning is correct, and DJGPP isn't supposed to know about
curl's printfs. Also not all curl code uses the curl printf. If we're allowing C99 types,
they will sooner or later occur in one of those calls.

DJGPP also caught non-printf cases, unrelated to curl's printf.

edit: FTR, I personally don't use or care for MS-DOS, but until curl is supporting
it, I regard it like any other supported platform. curl may drop support it, if DJGPP
is considered too annoying, buggy or turns out to have broken C99 support.
(It's gcc 12.2.0, seems unlikely, but possible.)

@bagder
Copy link
Member Author

bagder commented Jan 7, 2026

Though I think the warning is correct, and DJGPP isn't supposed to know about curl's printfs.

The printf() warnings have traditionally been useful since they detect mistakes and they do this because our printf() implementation is close enough to POSIX.

In these cases, the warnings are quite clearly not correct for our code. We know this because we can read the printf() implementation. We also know this because we can use this code on countless platforms and even on MS-DOS without using PRIu32. These are pointless warnings that add nothing for us.

Also not all curl code uses the curl printf. If we're allowing C99 types, they will sooner or later occur in one of those calls.

I'm again focusing on curl production code and there we always use our printf() code. And sure, if we use some other printf() somewhere else, that is another story.

@rsbeckerca
Copy link
Contributor

Using uint32_t is much kinder when dealing with multi-platform and multi-memory model compilation, IMO.

In particular, it turns 'unsigned long' into 'uint32_t' since the code
needs to build and run just as fine on Windows which has 32 bit longs,
so we know the code works with 32 bit field versions.

This makes Curl_easy 56 bytes smaller on my 64 bit Linux (maximized
build).
@bagder bagder force-pushed the bagder/urldata-uint branch from 5da0147 to 4bd448e Compare January 8, 2026 13:17
@bagder bagder closed this in e369161 Jan 8, 2026
@bagder bagder deleted the bagder/urldata-uint branch January 8, 2026 21:35
unsigned int creds_from:2; /* where is the server credentials originating
from, see the CREDS_* defines above */
uint32_t creds_from:2; /* where is the server credentials originating from,
see the CREDS_* defines above */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On OS400:
/home/MONNERAT/curl/lib/urldata.h, 1083.12: CZM0009(30) Bit field creds_from must be of type signed int, unsigned int

uint32_t field breaks compilation: it obviously is not an int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: IBM C header file stdint.h defines:

#ifndef _UINT32_T 
 #define _UINT32_T
 typedef unsigned long        uint32_t;
#endif

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, bitfields should not be done like that...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)
Thanks for your quick reaction

bagder added a commit that referenced this pull request Jan 10, 2026
Bug: #20209 (review)

Reported-by: Patrick Monnerat
Follow-up to e369161
bagder added a commit that referenced this pull request Jan 10, 2026
Bug: #20209 (review)

Reported-by: Patrick Monnerat
Follow-up to e369161
Closes #20244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants