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

getinfo: return sizes as curl_off_t #1511

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

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

@bagder bagder added the feature-window A merge of this requires an open feature window label May 25, 2017
@mention-bot
Copy link

@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
Copy link
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
Copy link
Member Author

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

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 Compare June 15, 2017 13:26
@bagder
Copy link
Member Author

bagder commented Jun 15, 2017

New version that uses _T as the new suffix instead.

@coveralls
Copy link

Coverage Status

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

@bagder
Copy link
Member Author

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 June 19, 2017 07:28
@jay jay removed the feature-window A merge of this requires an open feature window label Nov 21, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants