2
\$\begingroup\$

I want to read \n terminated, UTF-8 encoded lines from a network connection and process each full line. I cannot use fgets, because that function does not support nonblocking IO, which I want to use.

I currently have the following code, but it looks a bit too confusing to me. Some tips on how to simplify the code would be appreciated.

#include <array>
#include <string.h>

// Read lines from a non-blocking reader (e.g. an O_NONBLOCK socket).
// Every complete line is sent to a consumer function, without a newline or null terminator.
// Lines larger than BufSize are silently discarded.
//
// Reader should have approximately this signature:
//   ssize_t reader(void *buffer, size_t maxSize);
//   Should return 0 on EOF, -1 on error, otherwise the number of read bytes.
//   In practice, this should just call read() on a file descriptor.
// Consumer should have approximately this signature:
//   void consumer(const char *start, size_t size);
template<size_t BufSize>
class LineBuffer {
    std::array<char, BufSize> _buffer;
    size_t _bytesBuffered {0};
    size_t _bytesConsumed {0};
    bool _discardLine {false};
public:
    template<typename Reader, typename Consumer>
    size_t readLines(const Reader& reader, const Consumer& consumer)
    {
        char *buf = _buffer.data();
        while (true) {
            auto bytesRead = reader(buf + _bytesBuffered, _buffer.size() - _bytesBuffered);
            if (bytesRead <= 0) {
                return bytesRead;
            }
            _bytesBuffered += bytesRead;

            char* separator = nullptr;
            do {
                char* lineStart = buf + _bytesConsumed;
                separator = static_cast<char*>(memchr(
                    lineStart,
                    '\n',
                    _bytesBuffered - _bytesConsumed));

                if (separator) {
                    size_t lineLength = separator - lineStart;
                    if (!_discardLine) {
                        consumer(lineStart, lineLength);
                    }
                    _bytesConsumed += lineLength + 1;
                    _discardLine = false;
                }
            } while (separator);
            resetBuffer();
        }
    }
private:
    void resetBuffer() {
        size_t bytesRemaining = _bytesBuffered - _bytesConsumed;
        if (bytesRemaining == _buffer.size()) {
            // line too long
            _discardLine = true;
            _bytesBuffered = 0;
            _bytesConsumed = 0;
        } else if (bytesRemaining > 0 && _bytesConsumed > 0) {
            // move the last partial message to the front of the buffer, so a full-sized
            // message will fit
            memmove(_buffer.data(), _buffer.data() + _bytesConsumed, bytesRemaining);
            _bytesBuffered = bytesRemaining;
            _bytesConsumed = 0;
        }
    }
};

// Small test
#include <iostream>
#include <string>

LineBuffer<16> buffer;
void feed(const char *testData) {
    auto end = testData + strlen(testData);
    buffer.readLines(
        [&](void *buf, size_t maxSize) {
            auto size = std::min((ssize_t) maxSize, end - testData);
            memcpy(buf, testData, size);
            testData += size;
            return size;
        },
        [](const char *line, size_t size) {
            std::cout << std::string {line, size} << std::endl;
        });
}

int main() {
    // expected output: one, two, three, four, five, 0123456789abcde.
    feed("one\ntwo\n");
    feed("thr");
    feed("ee\n");
    feed("four\nThis line should be discarded, because it is too long.\n");
    feed("five\n0123456789abcde\n");
}
\$\endgroup\$
2
\$\begingroup\$
           if (bytesRead <= 0) {
                return bytesRead;
            }

How can an unsigned type be less than zero?

static_cast<char*>(memchr(

Why aren't you using std algorithms? You want to find the '\n' character in the text, right? That just std::find(begin_iterator, end_iterator, '\n'). No casting.

memmove(_buffer.data(), _buffer.data() + _bytesConsumed, bytesRemaining);

again, use std::copy or copy_n, not memmove.

For whatever functions you do use, include the proper headers for them. I see you have <string.h> but you should not use the C headers in C++. Use <cstring> instead.

⧺SL.io.50 Don't use endl.

If you are expecting this to be launched using async or similar, and it runs until the stream is closed or errored or whatever, then returning a value is not very useful: who are you returning it to? As noted earlier, it will always return 0, so it's useless to have this return value.


I don't understand how your loop works.

do {
  // read stuff
  if (separator) { ... }
while (separator);

I thought it would keep reading until it found the end-of-line, but this keeps looping until it doesn't read something with a linebreak.

I agree it's complex, because of your looping and condition structure. You have state variables that need to be understood in order to understand the flow. It could be written in a simpler way as a single loop with clearer conditions.

\$\endgroup\$
3
  • \$\begingroup\$ As I understand it from the comments at the start of the program, auto bytesRead = reader(...) is expected to return a ssize_t which is a signed type and can be negative. \$\endgroup\$
    – Martin R
    Oct 29 at 14:50
  • \$\begingroup\$ So the return type of readLines should match that, since it's the only return statement. \$\endgroup\$
    – JDługosz
    Oct 29 at 14:53
  • \$\begingroup\$ Yes, that is correct. (It was not apparent to me if your point is about the signed-ness of bytesRead or about the signed-ness of the return type of the function.) \$\endgroup\$
    – Martin R
    Oct 29 at 14:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.