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

add CURLSSLBACKEND_ALL enum #11903

Closed
wants to merge 1 commit into from

Conversation

TedLyngmo
Copy link
Contributor

[ssl]: add CURLSSLBACKEND_ALL enum for cleaner code and examples

This is to make code that now does (curl_sslbackend)-1 clearer
and easier to read:

/* list the available ones */
const curl_ssl_backend **list;
curl_global_sslset(CURLSSLBACKEND_ALL, NULL, &list);

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Also, you did not update symbols-in-versions so test 1119 fails.

include/curl/curl.h Outdated Show resolved Hide resolved
@TedLyngmo
Copy link
Contributor Author

symbols-in-versions

Thanks! What will the next version be? 8.3.1?

@bagder
Copy link
Member

bagder commented Sep 21, 2023

Thanks! What will the next version be? 8.3.1?

We are fairly sure it will become 8.4.0 so I think you can use that.

@jay
Copy link
Member

jay commented Sep 21, 2023

IMO curl_global_sslset(CURLSSLBACKEND_ALL, ... is confusing because it implies setting all backends. To me, it makes more sense as -1.

@bagder bagder added feature-window A merge of this requires an open feature window libcurl API labels Sep 21, 2023
@bagder
Copy link
Member

bagder commented Sep 21, 2023

CURLSSLBACKEND_ALL

Yeah, maybe *_NONE instead?

@TedLyngmo
Copy link
Contributor Author

TedLyngmo commented Sep 21, 2023

CURLSSLBACKEND_ALL

Yeah, maybe *_NONE instead?

CURLSSLBACKEND_NONE is already defined to 0. I used _ALL because it seemed to fit into the documentation:

/* list the available ones */
const curl_ssl_backend **list;
curl_global_sslset((curl_sslbackend)-1, NULL, &list);

Perhaps _ALL_AVAILABLE would be better?

@bagder
Copy link
Member

bagder commented Sep 21, 2023

CURLSSLBACKEND_NONE is already defined to 0. I used _ALL because it seemed to fit into the documentation:

Oh right. But maybe we should then rather change the documentation to use *NONE instead of -1? As far as I can see, that gives the same result...

@TedLyngmo
Copy link
Contributor Author

TedLyngmo commented Sep 21, 2023

CURLSSLBACKEND_NONE is already defined to 0. I used _ALL because it seemed to fit into the documentation:

Oh right. But maybe we should then rather change the documentation to use *NONE instead of -1? As far as I can see, that gives the same result...

Ah, well, that would certainly be even easier if it indeed does the same thing :-) I'm a bit worried about this note in the man-page that I missed before:

The backend can also be specified via the name parameter for a case insensitive match (passing -1 as id). If both id and name are specified, the name will be ignored.

That seems to imply that -1 and 0 are different somehow, but I haven't found how.

I added #11909 as an alternative to this PR.

[ssl]: add CURLSSLBACKEND_ALL enum for cleaner code and examples

This is to make code that now does `(curl_sslbackend)-1` clearer
and easier to read:

/* list the available ones */
const curl_ssl_backend **list;
curl_global_sslset(CURLSSLBACKEND_ALL, NULL, &list);

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
@TedLyngmo
Copy link
Contributor Author

Since #11909 has been approved, this PR is meaningless. Closing.

@TedLyngmo TedLyngmo closed this Sep 21, 2023
@TedLyngmo TedLyngmo deleted the CURLSSLBACKEND_ALL branch September 21, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-window A merge of this requires an open feature window libcurl API
Development

Successfully merging this pull request may close these issues.

None yet

3 participants