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

curl.h: change some enums to defines with L suffix #16482

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Feb 25, 2025

To help applications doing the right thing easier, change some enum values into defines with L suffixes so that they get the corect type easier when used with curl_easy_setopt().

To reduce the risk that this breaks the compile for any existing user, the previously provided enums are still provided, but the values to use are not defined by the enums.

@testclutch
Copy link

Analysis of PR #16482 at d21c4f66:

Test 503 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@bagder bagder marked this pull request as ready for review February 25, 2025 22:23
@github-actions github-actions bot added the CI Continuous Integration label Feb 25, 2025
@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

@vszakats look, I figured out some of the mysterious RTSP failures on macOS gcc-12. What's curious is that they only failed these particular builds...

@vszakats
Copy link
Member

vszakats commented Feb 25, 2025

@vszakats look, I figured out some of the mysterious RTSP failures on macOS gcc-12. What's curious is that they only failed these particular builds...

Wow, that's super nice! Those were puzzling. (edit: Ref: item 15 in c349bd6 #14097)

I wonder if there is any way to make these fall out early somehow.
Also why only macOS and why only gcc? It'd be interesting to see
how gcc-13 and gcc-14 are affected.

To help applications do the right thing easier, change some enum values
into defines with L suffixes so that they get the corect type (long)
easier when used with curl_easy_setopt(). This also fixes a few of our
own libtests.

To reduce the risk that this change breaks the compile for any existing
users, the previously provided enums are still provided, but the values
to use are not defined by the enums.

Closes #16482
@bagder bagder force-pushed the bagder/public-enum-define branch from b54099b to 9ea40e7 Compare February 25, 2025 22:58
@jay
Copy link
Member

jay commented Feb 25, 2025

To reduce the risk that this breaks the compile for any existing user, the previously provided enums are still provided, but the values to use are not defined by the enums.

I think it's unnecessary to keep the enums around. When would someone be using the enum type? In C++ ? They'd already have to change it for the other symbols then wouldn't they?

@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

I think it's unnecessary to keep the enums around

I figured this was the most conservative approach. The curl tool itself actually uses one of them and I took that as a hint that maybe there are a few other users too out there in the world who do. And keeping them is a small cost I think. If it helps not breaking a few builds out there.

@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

It'd be interesting to see how gcc-13 and gcc-14 are affected.

It seems reasonable to presume that they are/were. I mean this was just a plain bug in our tests that pass in ints where longs should be used.

@bagder bagder closed this in 2ec0037 Feb 26, 2025
@bagder bagder deleted the bagder/public-enum-define branch February 26, 2025 06:59
vszakats added a commit that referenced this pull request Mar 6, 2025
It fixes tests 1539, and 2402, 2404 (for non-Secure Transport), on macOS
with the gcc compiler.

Also unignore these tests in GHA/macos for non-secure transport.

Ref: c349bd6 #14097 (issue 15.)
Ref: 7b0240c #16539
Ref: 2ec0037 #16482

Closes #16580
vszakats added a commit that referenced this pull request Mar 6, 2025
To fix this test on macOS with the gcc compiler.

Also unignore test 1156 in GHA/macos.

Ref: c349bd6 #14097 (issue 15.)
Ref: 7b0240c #16539
Ref: 2ec0037 #16482

Closes #16579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmdline tool libcurl API
Development

Successfully merging this pull request may close these issues.

4 participants