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

tool_operate: fix type-limits compiler warning #1378

Merged
merged 2 commits into from Apr 5, 2017

Conversation

MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Apr 1, 2017

There's a "type-limits" warning with MinGW in tool_operate.c as a variable of type long is checked to be smaller than a 64-bit constant, which is always true for 32-bit long. Fixed this by doing the comparison only if long is at least 64 bits.

There are two commits: the first one only moves a part of the very long operate_do function 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.)

@mention-bot
Copy link

@mention-bot mention-bot commented Apr 1, 2017

@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.

@MarcelRaad
Copy link
Member Author

@MarcelRaad MarcelRaad commented Apr 1, 2017

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.

@bagder
Copy link
Member

@bagder bagder commented Apr 2, 2017

Oh right, this made me realize CURLINFO_FILETIME has a Y2038 problem even on 64bit windows since the long it returns is 32 bit there, while all other 64bit architectures have 64bit longs...

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)
Copy link
Member

@jay jay Apr 2, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@MarcelRaad MarcelRaad Apr 3, 2017

Choose a reason for hiding this comment

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

Thanks, good catch! Fixed.
I had tried to configure with -ansi -pedantic, but got some compile errors. Maybe I should look into that.

Copy link
Member Author

@MarcelRaad MarcelRaad Apr 3, 2017

Choose a reason for hiding this comment

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

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 added a commit to MarcelRaad/curl that referenced this issue Apr 3, 2017
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 added a commit to MarcelRaad/curl that referenced this issue Apr 3, 2017
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
}
#endif
}
setfiletime(filetime, outs.filename, config->global->errors);
Copy link
Member

@jay jay Apr 4, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@MarcelRaad MarcelRaad Apr 4, 2017

Choose a reason for hiding this comment

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

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 added a commit to MarcelRaad/curl that referenced this issue Apr 4, 2017
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Apr 4, 2017
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
jay
jay approved these changes Apr 5, 2017
MarcelRaad added 2 commits Apr 5, 2017
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 MarcelRaad merged commit b547fff into curl:master Apr 5, 2017
0 of 3 checks passed
@MarcelRaad MarcelRaad deleted the filetime_warning branch Apr 5, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants