Skip to content

Conversation

@futuarmo
Copy link
Contributor

LWG-3464 istream::gcount() can overflow, so in case, if _Chcount is negative, return std::numeric_limits<streamsize>::max()

Fixes #1480

@futuarmo futuarmo requested a review from a team as a code owner November 18, 2020 13:23
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Nov 18, 2020
Copy link
Contributor

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

I wouldn't mind tests for this, but I won't demand them.

@StephanTLavavej
Copy link
Member

Thanks for mentioning testing - while thinking about whether it would be practical to test this, I realized that I was confused by reading LWG-3464, which said:

On a 32-bit platform files larger than 2GB can overflow the counter

This might not apply to our implementation. We always use long long:

using streamsize = long long;

streamsize _Chcount; // the character count

STL/stl/inc/istream

Lines 602 to 604 in c2ab522

_NODISCARD streamsize __CLR_OR_THIS_CALL gcount() const { // get count from last extraction
return _Chcount;
}

There's no practical way to exceed 263 - 1, and thus no practical way to test this.

However, we are affected by #388 - I'm not sure if that's relevant here. I would guess not, since this is directly incrementing an integer, but I'm not certain.

So, I wonder whether we need to make any changes here at all. That said, I believe that the PR clearly doesn't harm correctness, and I believe that it won't harm performance either (the function can be inlined, and the branch is trivially predictable).

@StephanTLavavej StephanTLavavej merged commit 3703281 into microsoft:master Dec 3, 2020
@StephanTLavavej
Copy link
Member

Thanks for implementing this LWG issue and making the Status Chart happier (when it's updated next week)! 😸

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

Labels

LWG Library Working Group issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LWG-3464 istream::gcount() can overflow

5 participants