-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Ensure compiles on Fedora with GCC 12 #3132
Conversation
Fixes the problem when compiling with Gcc 12.2.1 20221121 and curl 7.87.0 that one gets In file included from /usr/include/curl/curl.h:3195, from turnrest.c:21: turnrest.c: In function ‘janus_turnrest_request’: turnrest.c:168:9: error: void value not ignored as it ought to be 168 | curl_easy_setopt(curl, api_http_get ? CURLOPT_HTTPGET : CURLOPT_POST, 1); | ^
Hi @bkmgit, I don't understand why the PR should solve the error returned by the compiiler
means that we are considering something void / returning void like it is not like that. Could you please clarify the purpose of the fix? |
Expect it maybe parsing or evaluation that is occurring incorrectly. |
If it would be helpful, can also add a config to do CI on Fedora Linux. |
I'd prefer not to start adding different linux distros on the CI. Anyway since the change is minor and harmless I don't see any reason not to merge this. |
I'd also like to first understand why a ternary operator causes the compiler to barf, before just merging, especially since we use those operators all over the place. Maybe it just needs wrapping in parenthesis to make the curl macros happy? |
Can examine using parentheses. The header file curl.h has not changed between curl 7.86.0 and curl 7.87.0 . Files with changes that have curl_setopt
and
|
@bkmgit did you have a chance to test with parentheses? |
curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L); | ||
}else{ | ||
curl_easy_setopt(curl, CURLOPT_POST, 1L); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small editorial notes: per our code style, you need a space between parentheses, so
if(api_http_get) {
and the else needs spaces too:
} else {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and suggestions. Probably no longer needed.
Closing in favor of #3138 which uses parentheses. |
#3131