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
Multiple fixes for issues found by Cppcheck #2631
Conversation
…n leads to undefined behaviour." in tests.
Remember 'make checksrc':
|
I should have thought of something like this |
lib/sendf.c
Outdated
int Curl_debug(struct Curl_easy *data, curl_infotype type, | ||
char *ptr, size_t size, | ||
int Curl_debug(struct Curl_easy *handle, curl_infotype type, | ||
char *data, size_t size, |
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.
I disagree with this. We basically "always" use 'data' for the Curl_easy handle pointer so it would introduce confusion by making this function an exception!
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.
so should I change .h file instead?
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.
yes I think so, thanks!
tests/libtest/lib1513.c
Outdated
@@ -58,7 +58,7 @@ int test(char *URL) | |||
easy_setopt(curl, CURLOPT_TIMEOUT, (long)7); | |||
easy_setopt(curl, CURLOPT_NOSIGNAL, (long)1); | |||
easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, progressKiller); | |||
easy_setopt(curl, CURLOPT_PROGRESSDATA, NULL); | |||
easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)NULL); |
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.
why is this desired?
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.
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 reference. So NULL can be an integer like 0 or 0L according to ANSI C, but POSIX says The macro shall expand to an integer constant expression with the value 0 cast to type void *.
But if we would take this to heart all calls to all curl_*_setopt()
functions with a NULL need that NULL typecasted and I would find that awefully annoying and ugly. The mere fact that no user ever had a problem with this construct is also a pretty strong indication that there really isn't any modern >32 bit system that uses a NULL that isn't a pointer.
A modern compiler would also warn on our code all over if NULL wasn't a pointer and we've never seen such compiler warnings.
All this taken into account, I think we should leave our NULL pointers non-casted, even in the setopt functions.
tests/libtest/lib516.c
Outdated
@@ -42,7 +42,7 @@ int test(char *URL) | |||
|
|||
/* First set the URL that is about to receive our POST. */ | |||
test_setopt(curl, CURLOPT_URL, URL); | |||
test_setopt(curl, CURLOPT_HTTPPOST, NULL); | |||
test_setopt(curl, CURLOPT_HTTPPOST, (struct curl_httppost *)NULL); |
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.
I don't think this helps...
… function leads to undefined behaviour." in tests." This reverts commit a7538cf.
1 test failed. But before last 3 commits it wasn't failing. They shouldn't change behavior at all. |
Correct, that test 1455 has showed up as a failure intermittently lately and is unrelated to your changes. |
Thanks! |
Cppcheck 1.83
Mostly reducing scope of variables.