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

getinfo: return sizes as curl_off_t #1511

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@bagder
Member

bagder commented May 25, 2017

This change introduces new alternatives for the existing six
curl_easy_getinfo() options that return sizes or speeds as doubles. The
new versions are named like the old ones but with an appended '2':

CURLINFO_CONTENT_LENGTH_DOWNLOAD2
CURLINFO_CONTENT_LENGTH_UPLOAD2
CURLINFO_SIZE_DOWNLOAD2
CURLINFO_SIZE_UPLOAD2
CURLINFO_SPEED_DOWNLOAD2
CURLINFO_SPEED_UPLOAD2

@mention-bot

This comment has been minimized.

mention-bot commented May 25, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jay, @captain-caveman2k and @philipc to be potential reviewers.

@jay

This comment has been minimized.

Member

jay commented May 25, 2017

This isn't a firm objection but I think that might be confusing to the user. Though I understand the func func2 pattern I don't think it would be thought of with macros like this, for example CURLINFO_SIZE_DOWNLOAD2 apply to a second download or what. It requires more cognitive processing. CURLINFO_SIZE_DOWNLOAD_AS_CURL_OFF_T
easier to understand? but longer and won't look very good in code. i don't know.. is there some major disadvantage to the user casting the double back to a curl_off_t for example, who's going above that.

@bagder

This comment has been minimized.

Member

bagder commented May 25, 2017

I agree completely on the names. I really don't like adding "2" to the names but I failed to come up with a nice naming scheme that doesn't also make the names extremely long... Would appending just _OFF_T be acceptable?

is there some major disadvantage to the user casting the double back to a curl_off_t for example, who's going above that

There isn't any major disadvantage, no, unless you use extremely large files. For me this is more about correctness, for a less surprising choice and for users not having to typecast the value to get it to a sensible type. These values are kept in curl_off_t internally already. (If I could remove the double returning ones I would, but we need to keep them for compatibility.)

@jay

This comment has been minimized.

Member

jay commented May 25, 2017

_OFF_T could imply off_t not curl_off_t and I think could lead to a mistake for that type would be more likely to lead to a mistake than 2.

There isn't any major disadvantage, no, unless you use extremely large files.

Yeah but just how big would those files have to be, unless you're transferring >8 petabytes (I'm basing this on 2^53 as max integer value once it's cast to double).

getinfo: return sizes as curl_off_t
This change introduces new alternatives for the existing six
curl_easy_getinfo() options that return sizes or speeds as doubles. The
new versions are named like the old ones but with an appended '_T':

CURLINFO_CONTENT_LENGTH_DOWNLOAD_T
CURLINFO_CONTENT_LENGTH_UPLOAD_T
CURLINFO_SIZE_DOWNLOAD_T
CURLINFO_SIZE_UPLOAD_T
CURLINFO_SPEED_DOWNLOAD_T
CURLINFO_SPEED_UPLOAD_T

@bagder bagder force-pushed the bagder/getinfo-return-sizes-off_t branch from 1d6b68e to a2565d0 Jun 15, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 15, 2017

New version that uses _T as the new suffix instead.

@coveralls

This comment has been minimized.

coveralls commented Jun 15, 2017

Coverage Status

Coverage decreased (-0.004%) to 73.158% when pulling a2565d0 on bagder/getinfo-return-sizes-off_t into efc83d6 on master.

@bagder

This comment has been minimized.

Member

bagder commented Jun 19, 2017

Intent announced to the list on June 16.

@bagder bagder closed this in 3b80d3c Jun 19, 2017

@bagder bagder deleted the bagder/getinfo-return-sizes-off_t branch Jun 19, 2017

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