[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: libcxx/trunk/include/istream:1223
+__state |= ios_base::badbit;
 return -1;
 }

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


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[wael yehia via Phabricator via cfe-commits]

w2yehia added inline comments.
Herald added a project: All.



Comment at: libcxx/trunk/include/istream:1222
 {
-this->setstate(ios_base::badbit);
+__state |= ios_base::badbit;
 return -1;

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?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D49863#1452154 , @ldionne wrote:

> In D49863#1452126 , @aprantl wrote:
>
> > It looks like this may have broken some LLDB data formatters:
> >  http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/
> >
> > http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/testReport/junit/lldb-Suite/functionalities_data-formatter_data-formatter-stl_libcxx_optional/TestDataFormatterLibcxxOptional_py/
> >  
> > http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/testReport/junit/lldb-Suite/functionalities_data-formatter_data-formatter-stl_libcxx_unordered/TestDataFormatterUnordered_py/
>
>
> I reverted in r357536 to give some time to figure what the problem is.


It turns out I think only r357531 had been applied and this is what broke the 
LLDB CI.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Louis Dionne via Phabricator via cfe-commits]

ldionne reopened this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D49863#1452126 , @aprantl wrote:

> It looks like this may have broken some LLDB data formatters:
>  http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/
>
> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/testReport/junit/lldb-Suite/functionalities_data-formatter_data-formatter-stl_libcxx_optional/TestDataFormatterLibcxxOptional_py/
>  
> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/testReport/junit/lldb-Suite/functionalities_data-formatter_data-formatter-stl_libcxx_unordered/TestDataFormatterUnordered_py/


I reverted in r357536 to give some time to figure what the problem is.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

It looks like this may have broken some LLDB data formatters:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/testReport/junit/lldb-Suite/functionalities_data-formatter_data-formatter-stl_libcxx_optional/TestDataFormatterLibcxxOptional_py/
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22934/testReport/junit/lldb-Suite/functionalities_data-formatter_data-formatter-stl_libcxx_unordered/TestDataFormatterUnordered_py/


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

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.




Comment at: libcxx/include/istream:365
 __input_arithmetic(basic_istream<_CharT, _Traits>& __is, _Tp& __n) {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
+ios_base::iostate __state = ios_base::goodbit;
+typename basic_istream<_CharT, _Traits>::sentry __s(__is);

lichray wrote:
> I like `auto` just FYI :)
You may like `auto`, but C++03 does not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.
Herald added a subscriber: jkorous.

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?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Zhihao Yuan via Phabricator via cfe-commits]

lichray accepted this revision.
lichray added inline comments.
This revision is now accepted and ready to land.
Herald added a subscriber: libcxx-commits.



Comment at: libcxx/include/istream:365
 __input_arithmetic(basic_istream<_CharT, _Traits>& __is, _Tp& __n) {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
+ios_base::iostate __state = ios_base::goodbit;
+typename basic_istream<_CharT, _Traits>::sentry __s(__is);

I like `auto` just FYI :)



Comment at: libcxx/include/istream:1005
 default:
-read(__s, _VSTD::min(__c, __n));
 break;

LOL


Repository:
  rCXX libc++

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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/dr.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).


Repository:
  rCXX libc++

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits