7
\$\begingroup\$

Some context: I have some existing code to populate network header structs from istreams. A motivating excerpt:

struct l2_eth_frame {
    using mac_address_t = std::array<std::uint8_t, 6>;
    mac_address_t dest, src;
    std::uint16_t type;
    std::uint32_t vlan;
};

template<typename StreamLike, typename T>
void read_ntoh(StreamLike &is, T &t){
    static_assert(std::is_integral<T>::value, "fixed width integer required");
    is.read(reinterpret_cast<char*>(&t), sizeof(T));

    uint8_t *p = reinterpret_cast<uint8_t*>(&t);
    std::reverse(p, p + sizeof(T));
}

template<class StreamLike>
l2_eth_frame read_l2_eth_frame(StreamLike &i)
{
    l2_eth_frame e;
    i.read(reinterpret_cast<char*>(&e.dest), sizeof(e.dest));
    i.read(reinterpret_cast<char*>(&e.src), sizeof(e.src));
    read_ntoh(i, e.type);
    if(e.type == 0x8100){
        i.read(reinterpret_cast<char*>(&e.vlan), sizeof(e.vlan));
    }
    return e;
}

int main() {
     std::ifstream ifs("test.pcap");
     auto eth = read_l2_eth_frame(ifs);
}

I'm now trying to reuse this code to also be able to generate network structs from live data (e.g. from a raw socket). To achieve this I wrote a simplistic array_view class and also an adaptor class that gives it read and ignore methods (like an istream) so that it can utilise static polymorphism in the read_{header_type} methods.

array_view

#pragma once

#include <cassert>
#include <memory>
#include <array>

template <class T>
class basic_array_view {

private:
    const T* array;
    std::size_t len;

public:
    static constexpr std::size_t npos = -1;

    basic_array_view() noexcept :
        array(nullptr),
        len(0)
    {}

    basic_array_view(const T* array, std::size_t len) noexcept :
        array(array),
        len(len)
    {}

    template<std::size_t N>
    basic_array_view(const T (& a)[N]) noexcept :
        array(std::addressof(a[0])),
        len(N)
    {}

    template<std::size_t N>
    basic_array_view(const std::array<T, N> &a) noexcept :
        array(a.data()),
        len(N)
    {}

    basic_array_view(const basic_array_view &other) = default;
    basic_array_view& operator=(const basic_array_view &other) = default;

    constexpr std::size_t size() const noexcept {
        return len;
    }

    const T& operator[](std::size_t pos) const noexcept {
        assert(pos < len);
        return *(array + pos);
    }

    const T* data() const noexcept {
        assert(len > 0);
        return array;
    }

    basic_array_view<T> subview_right(std::size_t pos=0, std::size_t count=npos){
        assert(pos <= len);
        return { array + pos, std::min(count, len - pos) };
    }
};

using array_view = basic_array_view<char>;

StreamLikeArrayView

#include "array_view.h"

class StreamLikeArrayView {

public:
    array_view av;

    template<class... Args>
    StreamLikeArrayView(Args&&... args) :
        av(std::forward<Args>(args)...) {}

    void read(char *dest, std::size_t len){
        std::copy(av.data(), av.data() + len, dest);
        av = av.subview_right(len, array_view::npos);
    }

    void ignore(std::size_t len){
        av = av.subview_right(len, array_view::npos);
    }
};

usage

std::array<char, 4096> buf;
const auto bytes_read = ::recv(sd, buf.data(), buf.size(), 0); //assume we have set up some raw socket sd
StreamLikeArrayView slav{buf.data(), bytes_read};
const auto eth = read_l2_eth_frame(slav);

I'm particularly interested in a review of the StreamLikeArrayView abstraction, and whether there's some superior way to reuse the parsing code across istreams and arrays.

\$\endgroup\$
  • \$\begingroup\$ if anyone can suggest a more coherent title, please let me know, my brain failed me there. \$\endgroup\$ – cloakedlearning Aug 31 '16 at 16:03
1
+200
\$\begingroup\$

Could be more generic

Let's consider one of your constructors:

template<std::size_t N>
basic_array_view(const std::array<T, N> &a) noexcept :
    array(a.data()),
    len(N)
{}

I don't see any particularly good reason this should/must be restricted to being used with an std::array. I think I'd write the code more like this:

template<class Container>
basic_array_view(Container const &a) noexcept :
    array(a.data()),
    len(a.length())
{}

Now it can be used with an std::array or other containers like std::vector, std::deque, etc.

Incomplete

The basic point of something like an array_view is (at least usually) to provide the same interface as existing collections. It does provide a substantial part of that, but it's missing a number of things like as the normal typedefs for value_type, reference_type, iterator_type, and so on. It's also missing iterator access to the contents; given that you're dealing with a contiguous array, that can be provided quite trivially (begin() and end() that return pointers to beginning and one past the end respectively). The latter would allow use with standard algorithms and such as well.

Naming

I'd try to pick one naming convention and stick with it. Either snake_case throughout or PascalCase throughout--but combining array_view with StreamLikeArrayView is somewhat jarring.

I also think subview_right is kind of a poor name--it seems as if subview is entirely adequate.

Duplication

In some ways this is the biggest question of all. You could get the functionality of your StreamLikeArrayView by simply dumping the data into a stringstream, and letting your existing code read from there. The (primary?) gain is that you avoid copying the data as you would with a normal stringstream.

If your real intent is to provide functionality like a stringstream's, but avoiding copying by having it use a buffer that isn't owned by the streambuf, then I think it would be better to provide that more directly:

struct array_buffer : public std::streambuf {
    array_buffer(char const *begin, char const *end) {
        char *b = const_cast<char *>(begin);
        char *e = const_cast<char *>(end);

        setg(b, b, e);
    }

    template <class Container>
    array_buffer(Container const &c) : array_buffer(&*std::begin(c), &*std::end(c))
    { }
};

[I should add: this is also somewhat incomplete--it doesn't try to support all possible stream buffer operations, but it supports at least as much as the classes above.]

To go with that, we can provide a stream class to save the user some work in constructing a stream based on our buffer type:

class buffer_stream : public std::istream {
    array_buffer buf;
public:
    template<class... Args>
    buffer_stream(Args&&... args) : buf(std::forward<Args>(args)...), std::istream(&buf)
    { }
};

To go with that, some demo code to show how we can build an istream of this type from any of a number of sources, and how the usual ways of reading from an istream work with this istream:

int main() {
    char input[] = "This is some input";

    // Create a buffer stream reading from an array
    buffer_stream b(input);

    std::string s;

    // Do some normal stream reading from it:
    while (b >> s)
        std::cout << s << "\n";

    // Create another from an `std::array`, read some from it:
    // Sorry, but I'm too lazy to hand-craft a very interesting array here:
    std::array<char, 3> more_input { 65, 66, 67 };

    buffer_stream c(more_input);

    while (c >> s)
        std::cout << s << "\n";

    // Create one from a string and read from it:
    std::string still_more_input { "This is the last piece of input" };

    buffer_stream d(still_more_input);

    // Read the first few words:
    char buffer[11];
    d.read(buffer, sizeof(buffer));
    std::cout.write(buffer, sizeof(buffer));
    std::cout << "\n";

    // Skip " last "
    d.ignore(6);

    // Then read the remainder into a string:
    std::getline(d, s);
    std::cout << s << "\n";
}

The obvious advantage of providing a class derived from istream is that it works with essentially everything that reads from an istream, not just with a couple of your own select functions that template the stream-like object, and restrict themselves to two specific ways of reading from the stream.

The obvious disadvantage of this approach is that is probably adds more overhead, and some people (especially anybody who hasn't delved into streams, buffers, etc.) may find it harder to understand. There's also the fact that by being a stream buffer class, people may assume this supports everything that any stream buffer might, which is a bit more than it actually delivers.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much for your answer. Now it's been pointed out it seems obvious that it's better to derive from a stream rather than create some weird Frankenstein composition. Can't believe I missed it. \$\endgroup\$ – cloakedlearning Sep 13 '16 at 16:17

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.