Page MenuHomePhabricator

[libc++] split `<istream>` header
AbandonedPublic

Authored by huixie90 on Sep 9 2022, 1:34 AM.

Details

Reviewers
philnik
var-const
ldionne
Group Reviewers
Restricted Project
Summary

<istream> defines basic_istream and basic_iostream. The basic_iostream brings all the dependencies to <ostream>.
While implementing <ranges>'s std::views::istream, we need the declaration of basic_istream and all its member >> and free >>,
because the concept checks the validity of >>. To avoid adding too many dependencies to <ranges>, it would be good idea to split
basic_istream and basic_iostream. Also, we only need the declarations of >> to pass the concept check, we don't need the extra
dependencies coming from the implementations, namely <locale> and its indirect dependencies (__locale isn't enough). Luckily, the basic_istream's member definition is out of line, so there is not much work to do
other than split the files. Also there is not much SFINAE going so keeping declaration and definition separate won't cause too much code duplication

Diff Detail

Event Timeline

huixie90 created this revision.Sep 9 2022, 1:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 1:34 AM
Herald added a subscriber: mgorny. · View Herald Transcript
huixie90 published this revision for review.Sep 11 2022, 4:39 AM
huixie90 edited the summary of this revision. (Show Details)
huixie90 added reviewers: philnik, var-const, ldionne.
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2022, 4:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
huixie90 retitled this revision from split `<istream>` header to [libcxx] split `<istream>` header.Sep 11 2022, 8:47 AM
huixie90 retitled this revision from [libcxx] split `<istream>` header to [libc++] split `<istream>` header.

The approach we generally take is to create a libcxx/include/__fwd/foo.h header. Would that work here?

libcxx/include/__istream/basic_iostream.h
272

No newline! Please add one!

The approach we generally take is to create a libcxx/include/__fwd/foo.h header. Would that work here?

Do you mean by creating a header that only contains the forward declaration like

template <class _Chart>
class basic_istream;

But the std::ranges::istream_view class itself has a constraint that >> needs to be valid

template <class _Val, class _CharT, class _Traits>
concept __stream_extractable = requires(basic_istream<_CharT, _Traits>& __is, _Val& __t) { __is >> __t; };

template <movable _Val, class _CharT, class _Traits = char_traits<_CharT>>
  requires default_initializable<_Val> && __stream_extractable<_Val, _CharT, _Traits>
class basic_istream_view;

That means the declaration of basic_istream_view already requires all the member >> and free >> from the basic_istream. So only a forward declaration of basic_istream isn't enough.

And this is the main reason I made the basic_istream.h header contains all the >> declarations.

I think this can be pretty much abandoned and the forward declaration of basic_istream folded into the istream_view patch itself.

libcxx/include/__istream/basic_istream.h
1

In light of the survey of other implementations we just did during live review (https://godbolt.org/z/G6EbqvY57), I suggest we:

  1. Add a __fwd/basic_istream.h header that forward declares the class itself, but nothing else.
  2. Include __fwd/basic_istream.h from istream_view.h, but not all of iostream or even a clever subset of it.

The result will be that users can't use istream_view without including <istream> themselves, however that's what other implementations do. And the alternative would be to pull in large parts of iostream and the localization library, and that's unacceptable.

huixie90 abandoned this revision.Oct 5 2022, 11:56 PM