This is an archive of the discontinued LLVM Phabricator instance.

[istream] Fix error flags and exceptions propagated from input stream operations
ClosedPublic

Authored by ldionne on Jul 26 2018, 9:25 AM.

Details

Summary

Before this patch, we would only ever throw an exception if the badbit
was set on the stream. The Standard is currently very unclear on how
exceptions should be propagated and what error flags should be set by
the input stream operations. This commit changes libc++ to behave under
a different (but valid) interpretation of the Standard. This interpretation
of the Standard matches what other implementations are doing.

I will submit a paper in San Diego to clarify the Standard such that the
interpretation used in this commit (and other implementations) is the only
possible one.

https://bugs.llvm.org/show_bug.cgi?id=21586
https://bugs.llvm.org/show_bug.cgi?id=15949
rdar://problem/15347558

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Jul 26 2018, 9:25 AM

This is a very tricky change since it touches all the input stream operations, both formatted and unformatted. However, I think it's an important change as it fixes extremely broken behavior. The paper I'm working on to fix this in the Standard is https://github.com/ldionne/wg21/blob/4d963b488182b96479636c252695542671fd5e41/generated/drxxxx.pdf. Perhaps we should wait for the paper to make its way through the Standard before committing this, but in any case the new behavior of libc++ should now match that of libstdc++ and MSVC (couldn't test with those libraries, though).

lichray accepted this revision.Nov 8 2018, 11:20 AM
lichray added a subscriber: lichray.
lichray added inline comments.
libcxx/include/istream
365 ↗(On Diff #157506)

I like auto just FYI :)

1005 ↗(On Diff #157506)

LOL

This revision is now accepted and ready to land.Nov 8 2018, 11:20 AM

I would like to merge this even though http://wg21.link/p1264 has not been voted into C++ yet, because this is arguably a bug and libc++ differs from other stdlibs in this regard. Once http://wg21.link/p1264 is merged into C++ in one way of another, libc++ will simply have nothing to do.

@mclow.lists Are you OK with that?

ldionne updated this revision to Diff 193342.Apr 2 2019, 12:27 PM

Rebase on top of master.

Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 12:27 PM
mclow.lists accepted this revision.Apr 2 2019, 1:28 PM

I'm OK with this. I think, strictly speaking, that libc++ is following the standard, and everyone else is not - but what everyone else is doing makes sense.

libcxx/include/istream
365 ↗(On Diff #157506)

You may like auto, but C++03 does not.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 2:43 PM
This revision is now accepted and ready to land.Apr 2 2019, 3:22 PM
ldionne updated this revision to Diff 193900.Apr 5 2019, 9:28 AM

Rebase on top of master.

This revision was automatically updated to reflect the committed changes.
w2yehia added a subscriber: w2yehia.Aug 5 2022, 3:43 PM
w2yehia added inline comments.
libcxx/trunk/include/istream
1222

Hi @ldionne, you're setting the local __state and returning, while before we calling this->setstate.
I see you added this->setstate(__state); in the bottom.
It seems this is a bug?

Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 3:43 PM
hubert.reinterpretcast added inline comments.
libcxx/trunk/include/istream
1223

@ldionne, another dubious aspect of this patch is that it initializes __r to 0 and never sets it. Seems like this line should have been to set __r and not to return directly.