tool_operate: fix type-limits compiler warning #1378
Conversation
|
@MarcelRaad, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @bagder and @captain-caveman2k to be potential reviewers. |
|
Ah, seems like GitHub always shows the diff with "compare whitespace changes" - the actual changes of the second commit are best visible with "ignore whitespace changes" as the indentation was changed. |
|
Oh right, this made me realize |
src/tool_operate.c
Outdated
| saving time offset and since it's GMT that is bad behavior. When we have | ||
| access to a 64-bit type we can bypass utime and set the times directly. */ | ||
| #if defined(WIN32) && (CURL_SIZEOF_CURL_OFF_T >= 8) | ||
| #if (CURL_SIZEOF_LONG >= 8) |
There was a problem hiding this comment.
We can't have mixed declarations so HANDLE hfile would have to be declared before this line. also for all cases we expect filetime to be >= 0, and while I see the caller is checking that it is also appropriate to check in this function at the beginning if(filetime < 0) return
There was a problem hiding this comment.
Thanks, good catch! Fixed.
I had tried to configure with -ansi -pedantic, but got some compile errors. Maybe I should look into that.
There was a problem hiding this comment.
I also moved the if(filetime >= 0) into the function. (I didn't do the if(filetime < 0) return because then the HANDLE declaration would again be not the first statement in the block.)
MarcelRaad
force-pushed
the
filetime_warning
branch
from
eef77b7
to
68bca76
Compare
MinGW complains: tool_operate.c:197:15: error: comparison is always true due to limited range of data type [-Werror=type-limits] Fix this by only doing the comparison if 'long' is large enough to hold the constant it is compared with. Closes curl#1378
MarcelRaad
force-pushed
the
filetime_warning
branch
from
68bca76
to
f944d71
Compare
MinGW complains: tool_operate.c:197:15: error: comparison is always true due to limited range of data type [-Werror=type-limits] Fix this by only doing the comparison if 'long' is large enough to hold the constant it is compared with. Closes curl#1378
src/tool_operate.c
Outdated
| } | ||
| #endif | ||
| } | ||
| setfiletime(filetime, outs.filename, config->global->errors); |
There was a problem hiding this comment.
Please put back the filetime >= 0 check here, having the check in both places is appropriate I think. Otherwise this looks fine. Regarding your question about the commit messages, both should have a blank line followed by a reference to this issue. So you can do that with a Ref: xxx line or a Closes xxx line, or if you were responding to someone else's report a Bug: xxx line followed by a Reported-by: xxx line, etc. The important part is there is always a link back to the issue(s) so nobody has to hunt it down in the future if they need more info on it.
There was a problem hiding this comment.
Thanks a lot! I've put back the check at the call site, amended the commit messages with the Ref:/Closes, moved the if(filetime >= 0) to the first commit, and rebased on current master.
MarcelRaad
force-pushed
the
filetime_warning
branch
from
f944d71
to
87808b3
Compare
MinGW complains: tool_operate.c:197:15: error: comparison is always true due to limited range of data type [-Werror=type-limits] Fix this by only doing the comparison if 'long' is large enough to hold the constant it is compared with. Closes curl#1378
MinGW complains: tool_operate.c:197:15: error: comparison is always true due to limited range of data type [-Werror=type-limits] Fix this by only doing the comparison if 'long' is large enough to hold the constant it is compared with. Closes curl#1378
MarcelRaad
force-pushed
the
filetime_warning
branch
from
87808b3
to
b547fff
Compare
There's a "type-limits" warning with MinGW in tool_operate.c as a variable of type
longis checked to be smaller than a 64-bit constant, which is always true for 32-bitlong. Fixed this by doing the comparison only iflongis at least 64 bits.There are two commits: the first one only moves a part of the very long
operate_dofunction to a new function and the second one is the actual warning fix. So the "Closes" should be in the commit message for the second commit, right?(I'm currently trying to get the autobuilds page a little less red and started with the warnings section.)