Multiple fixes for issues found by Cppcheck #2631

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
2 participants
@Nekto89
Contributor

Nekto89 commented Jun 3, 2018

Cppcheck 1.83
Mostly reducing scope of variables.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 3, 2018

Member

Remember 'make checksrc':

./lib506.c:134:2: warning: Trailing whitespace (TRAILINGSPACE)
   
  ^
./lib512.c:39:2: warning: Trailing whitespace (TRAILINGSPACE)
   
  ^
./lib556.c:74:4: warning: Trailing whitespace (TRAILINGSPACE)
     
    ^
./lib586.c:102:2: warning: Trailing whitespace (TRAILINGSPACE)
   
  ^
Member

bagder commented Jun 3, 2018

Remember 'make checksrc':

./lib506.c:134:2: warning: Trailing whitespace (TRAILINGSPACE)
   
  ^
./lib512.c:39:2: warning: Trailing whitespace (TRAILINGSPACE)
   
  ^
./lib556.c:74:4: warning: Trailing whitespace (TRAILINGSPACE)
     
    ^
./lib586.c:102:2: warning: Trailing whitespace (TRAILINGSPACE)
   
  ^
@Nekto89

This comment has been minimized.

Show comment
Hide comment
@Nekto89

Nekto89 Jun 3, 2018

Contributor

I should have thought of something like this
git ls-files -z -- '.c' '.h' | xargs -0 sed -i 's/ +$//'
instead of manually fixing files from error logs

Contributor

Nekto89 commented Jun 3, 2018

I should have thought of something like this
git ls-files -z -- '.c' '.h' | xargs -0 sed -i 's/ +$//'
instead of manually fixing files from error logs

lib/sendf.c
-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,

This comment has been minimized.

@bagder

bagder Jun 3, 2018

Member

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!

@bagder

bagder Jun 3, 2018

Member

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!

This comment has been minimized.

@Nekto89

Nekto89 Jun 4, 2018

Contributor

so should I change .h file instead?

@Nekto89

Nekto89 Jun 4, 2018

Contributor

so should I change .h file instead?

This comment has been minimized.

@bagder

bagder Jun 6, 2018

Member

yes I think so, thanks!

@bagder

bagder Jun 6, 2018

Member

yes I think so, thanks!

tests/libtest/lib1513.c
@@ -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);

This comment has been minimized.

@bagder

bagder Jun 3, 2018

Member

why is this desired?

@bagder

bagder Jun 3, 2018

Member

why is this desired?

This comment has been minimized.

@bagder

bagder Jun 6, 2018

Member

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.

@bagder

bagder Jun 6, 2018

Member

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
@@ -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);

This comment has been minimized.

@bagder

bagder Jun 3, 2018

Member

I don't think this helps...

@bagder

bagder Jun 3, 2018

Member

I don't think this helps...

Nekto89 added some commits Jun 4, 2018

Revert "Fix "Passing NULL after the last typed argument to a variadic…
… function leads to undefined behaviour." in tests."

This reverts commit a7538cf.
@Nekto89

This comment has been minimized.

Show comment
Hide comment
@Nekto89

Nekto89 Jun 5, 2018

Contributor

1 test failed. But before last 3 commits it wasn't failing. They shouldn't change behavior at all.

Contributor

Nekto89 commented Jun 5, 2018

1 test failed. But before last 3 commits it wasn't failing. They shouldn't change behavior at all.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 11, 2018

Member

Correct, that test 1455 has showed up as a failure intermittently lately and is unrelated to your changes.

Member

bagder commented Jun 11, 2018

Correct, that test 1455 has showed up as a failure intermittently lately and is unrelated to your changes.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 11, 2018

Member

Thanks!

Member

bagder commented Jun 11, 2018

Thanks!

@bagder bagder closed this in c45360d Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment