IOStreamBuf (istream/basic_streambuf adaptor for IOBuf) by tedjp · Pull Request #554 · facebook/folly

tedjp · 2017-03-01T00:40:35Z

Allows chained IOBufs to be read via std::istream without having to coalesce them into a single contiguous buffer first.

Works with any single-byte type, eg. char or uint8_t. Not currently usable on multi-byte types like wchar_t due to the extra complexity involved in recomposing multi-byte characters that are split across IOBuf boundaries. (static_assert prevents use with multi-byte types.)

Currently read-only; cannot be used with a std::ostream.

Depends the io/test/Makefile.am changes in #550. I've included the two necessary commits in this pull request. Happy to resubmit this after #550 completes review if that's preferred.

There are probably good performance reasons to avoid using this in general — and perhaps that should be noted in the header — but it could be useful in conjunction with APIs that can read from an istream. My use for this was with Proxygen: passing an incoming message body (chained IOBuf) to a 3rd party API.

The use of a .tcc file for the implementation of most of the templated functions is unusual for Folly but I wanted to keep them out of the general header. It could easily be renamed to …-impl.h or whatever the preferred style is.

It takes about 4 minutes; the rest of the suite takes around 10 seconds. Still builds during `make check` and can be executed manually.

Allows chained IOBufs to be read via std::istream without having to coalesce them into a single contiguous buffer first. Works with any single-byte type, eg. `char` or `uint8_t`. Not currently usable on multi-byte types like `wchar_t` due to the extra complexity involved in recomposing multi-byte characters that are split across IOBuf boundaries. (static_assert prevents use with multi-byte types.) Currently read-only; cannot be used with a `std::ostream`.

facebook-github-bot · 2017-03-03T22:56:16Z

@Orvid has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Orvid · 2017-03-03T22:56:37Z

-inl.h is the preferred style for Folly.

Orvid

Via @yfeldblum:

A bunch of style nits to make this conform to the standard Folly style. I mention each nit category only once, but each mention refers implicitly to many occurrences in that category.

Orvid · 2017-03-14T18:14:51Z

@@ -0,0 +1,264 @@
+#include <folly/Likely.h>


Via @yfeldblum:

Missing an include guard (#pragma once preferably) and a license header.

Orvid · 2017-03-14T18:15:34Z

+class IOStreamBuf : public std::basic_streambuf<CharT, Traits> {
+  // Due to having to merge single-byte subsets of CharT across IOBuf
+  // boundaries, prevent the use of IOStreamBuf on multi-byte types for now.
+  static_assert(sizeof(CharT) == 1,


Via @yfeldblum:

Style. If the arguments must be broken across lines, then each one goes on its own line, and each one is indented +4. Here and elsewhere.

Orvid · 2017-03-14T18:16:49Z

+   * must ensure that the IOBuf provided lasts at least as long as the
+   * IOStreamBuf.
+   */
+  IOStreamBuf(const folly::IOBuf* head);


Via our internal linter:

Single-argument constructor IOStreamBuf(const folly::IOBuf* head) may inadvertently be used as a type conversion constructor. Prefix the function with the explicit keyword to avoid this, or add an /* implicit */ comment to suppress this warning.

In this case, marking it as explicit is the correct solution.

Orvid · 2017-03-14T18:17:27Z

+   */
+  IOStreamBuf(const folly::IOBuf* head);
+
+  IOStreamBuf(const IOStreamBuf&) = default;


Via @yfeldblum:

Style. Prefer const-on-the-right in new code. IOStreamBuf const&. Here and elsewhere.

Orvid · 2017-03-14T18:18:16Z

+
+  IOStreamBuf(const IOStreamBuf&) = default;
+  IOStreamBuf& operator=(const IOStreamBuf&) = default;
+  void swap(IOStreamBuf<CharT,Traits>&);


Via @yfeldblum:

Style. , (with space) between template arguments. Here and elsewhere.

Orvid · 2017-03-14T18:20:01Z

+
+}
+
+#include "IOStreamBuf-inl.h"


Via @yfeldblum:

Style. Include with the full include path, ie. folly/io/IOStreamBuf-inl.h.

Orvid · 2017-03-14T18:20:51Z

+template <typename CharT, typename Traits>
+typename IOStreamBuf<CharT,Traits>::int_type
+IOStreamBuf<CharT,Traits>::pbackfail(int_type c) {
+  if (this->gptr() != this->eback())


Via @yfeldblum:

Style. if requires {}. Here and elsewhere.

Orvid · 2017-03-14T18:21:31Z

+  const IOBuf* prev = gcur_;
+  do {
+    prev = prev->prev();
+  } while (prev->length() == 0 && prev != head_);


Via @yfeldblum:

Can also use std::find_if here.

Looks like IOBuf::Iterator is a forward-only iterator whereas this code needs to iterate backwards to the head. Furthermore it needs access to the IOBuf object itself but the IOBuf::Iterator only exposes a ByteRange (ie. the data within the IOBuf).

Orvid · 2017-03-14T18:23:07Z

+
+template <typename CharT, typename Traits>
+typename IOStreamBuf<CharT,Traits>::pos_type
+IOStreamBuf<CharT,Traits>::seekoff(off_type off,


Via @yfeldblum:

This is very long. At the very least, split it into three private member functions for the three possible values of way, since the implementations for the three code paths share no code.

Orvid · 2017-03-14T18:23:42Z

+iobuf_cursor_test_LDADD = $(ldadd)
+
+compression_test_SOURCES = CompressionTest.cpp
+compression_test_LDADD = $(top_builddir)/libfolly.la \


Via @yfeldblum:

Can drop the first entry to the next line, for style.

tedjp · 2017-03-17T20:25:22Z

Thanks for all the feedback. I'll send an update when time permits.

Avoids unintentional implicit construction.

Adjusted indentation of each line to a single tab to match top-level Makefile.am.

No functional code change; just refactoring.

facebook-github-bot

Orvid has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

tedjp added 3 commits February 28, 2017 17:01

Re-enable io tests

8476392

Disable compression-test by default

e239538

It takes about 4 minutes; the rest of the suite takes around 10 seconds. Still builds during `make check` and can be executed manually.

facebook-github-bot added CLA Signed GH Review: review-needed Import Started labels Mar 1, 2017

Rename IOStreamBuf.tcc -> IOStreamBuf-inl.h

d3e2223

Orvid suggested changes Mar 14, 2017

View reviewed changes

Orvid added GH Review: needs-revision and removed GH Review: review-needed labels Apr 11, 2017

tedjp added 11 commits April 26, 2017 12:22

Add copyright, license header & include guard

3aed8e8

Add braces around one-line if/for bodies

0d08f8e

Make IOStreamBuf constructor explicit

07523d5

Avoids unintentional implicit construction.

const after type

f4b2157

Remove virtual from methods that explicitly override

52ef076

Add space between template parameters

69ee124

Use full folly include path <>

b69d6a8

Start multi-like Makefile variables on a new line

5dbc66a

Adjusted indentation of each line to a single tab to match top-level Makefile.am.

Split seekoff into separate functions for each direction

dc7a34e

No functional code change; just refactoring.

Separate args onto new lines w/ 4-space indent

0e59963

Merge remote-tracking branch 'facebook/master' into iostreambuf-fb

05c7cfb

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Apr 28, 2017

facebook-github-bot reviewed Sep 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

IOStreamBuf (istream/basic_streambuf adaptor for IOBuf)#554

IOStreamBuf (istream/basic_streambuf adaptor for IOBuf)#554
tedjp wants to merge 15 commits into
facebook:mainfrom
tedjp:iostreambuf-fb

tedjp commented Mar 1, 2017

facebook-github-bot commented Mar 3, 2017

Orvid commented Mar 3, 2017

Orvid left a comment

Orvid Mar 14, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

tedjp Apr 26, 2017

yfeldblum Apr 27, 2017

Orvid Mar 14, 2017

Orvid Mar 14, 2017

tedjp commented Mar 17, 2017

facebook-github-bot left a comment

Labels

4 participants


		}

		#include "IOStreamBuf-inl.h"

Uh oh!

Conversation

tedjp commented Mar 1, 2017

facebook-github-bot commented Mar 3, 2017

Orvid commented Mar 3, 2017

Orvid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedjp commented Mar 17, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

Labels

4 participants