Details
- Reviewers
Mordante - Group Reviewers
Restricted Project - Commits
- rG61d2a9b3ea10: [libc++] Implement istringstream members of P0408R7 (Efficient Access to…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks! This look quite good, a few small issues.
libcxx/include/sstream | ||
---|---|---|
813 | I prefer the wording of the standard, I know the existing code doesn't do that but let's do it for the new code. | |
863 | This function is not constrained in the Standard. Why is it needed? | |
libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp | ||
28 | I would avoid std::ios_base::in in these tests if possible. This should be automatically be set. | |
libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp | ||
33 | Is the moved from state specified? |
libcxx/include/sstream | ||
---|---|---|
863 | If we omit this constraint, we get: call to member function 'str' is ambiguous ss.str(" 789"); I think the standard should include it. | |
libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp | ||
28 | Why? What to use instead of std::ios_base::in ? |
libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp | ||
---|---|---|
33 | It is:
|
libcxx/include/sstream | ||
---|---|---|
813 | This item is not done. | |
863 | Thanks. I want to look at this locally. If this indeed is needed it looks like a defect in the Standard. When this is a bug in the Standard we should file an LWG issue and mention that in the code. | |
libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp | ||
28 | When we don't set it we can test whether the in flag is set. I would set the binary flag. | |
libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp | ||
33 | Thanks! |
s/&/std::addressof/
s/std:ios_base::in/std::ios_base::binary/
libcxx/include/sstream | ||
---|---|---|
813 | Sorry, I'm still learning Phabricator. It's a little confusing that both "arc diff" and "Submit" on the web submit the "Done" flags. |
libcxx/include/sstream | ||
---|---|---|
863 | This is correct. I got reminded the Equivalent to wording "inherits" the constrains. |
Can you rebase the patch to make sure the CI is green. Note in Phabricator you can created stacked patches, which allows you to continue working on the other patches while they are reviewed.
Rebased without conflicts.
I also need to apply the https://reviews.llvm.org/D155185 workaround to istringstream.
If https://reviews.llvm.org/D154454 lands first, I can edit https://reviews.llvm.org/D155185
If https://reviews.llvm.org/D155185 lands first, I can edit https://reviews.llvm.org/D154454
I prefer the wording of the standard, I know the existing code doesn't do that but let's do it for the new code.