Variadic Datablocks

Question

I would like to have my variadic template class reviewed.

First some explanation what it should do:

I'm writing an application where I read in blocks of data.

I know before i read in the data which formats to expect.

For example:

   ID123.EnableValve := FALSE; 
   ID123.Position1 := 0; 
   ID123.Position2 := 2; 
   ID123.Position3 := 9;

Or:

   UDT30.EnableValve := FALSE; 
   UDT30.This := 0; 
   UDT30.is := L#2; 
   UDT30.an := 9; 
   UDT30.Example := TRUE;

These data blocks share the following similarities:

Each line starts with an id (e.g. UDT30 or ID123).
This Device Id must be the same on each line. Otherwise we read a corrupt data block.
Then a type name follows (e.g. EnableValve or Position3).
The type names are unique. That means in one data block there is never the same type twice (e.g. Position1 does not occur two times).
Then always the string := follows.
Add the end the value follows (e.g. TRUE, 0, L#).
The count of lines is flexible (e.g. length = 4 or 5).

I want to do the following with the datablocks:

Reading in datablocks (from std::istream)
Access a specific row and modify its value(e.g. ID123.EnableValve := FALSE; becomes ID123.EnableValve := TRUE;
Writing the datablock back (to std::ostream)

Also it must be possible to:

construct a new datablock out of Types
write the datablock (to std::osstream)

I want to inherit from this template and restrict the use of set and get for the rows. The inherited classes can decide with the protected set / get which lines they want to modify and which are just read only.

I used Google Test with Visual 2017 to create unit tests for the template. Here you can see what the template can do.

Variadic_templatesTest.cpp

#include "pch.h"

#include "..\variadic_templates\Fake_types.h"

// hack to test protected methods in variadic datablock
// is there a better way??
#define protected public

#include "..\variadic_templates\Variadic_datablock.h"

#include <sstream>

TEST(Variadic_templatesTest, DefaultConstructor) {

    Variadic_datablock t;
    EXPECT_EQ(t.get_id(), std::string{});
}

TEST(Variadic_templatesTest, Constructor) {

    std::string id{ "ID123" };
    EnableValve enableValve{ "TRUE" };
    Position1 position1{ "0" };
    Position2 position2{ "2" };
    Position3 position3{ "3" };

    Variadic_datablock<EnableValve, Position1, Position2, Position3> t{
        id,
        enableValve,
        position1,
        position2,
        position3
    };

    EXPECT_EQ(t.get_id(), id);
    EXPECT_EQ(t.get_element<EnableValve>().m, enableValve.m);
    EXPECT_EQ(t.get_element<Position1>().m, position1.m);
    EXPECT_EQ(t.get_element<Position2>().m, position2.m);
    EXPECT_EQ(t.get_element<Position3>().m, position3.m);
}

TEST(Variadic_templatesTest, SetElement) {

    std::string id{ "ID123" };
    EnableValve enableValve{ "TRUE" };

    Variadic_datablock<EnableValve> t{
        id,
        enableValve,
    };

    EXPECT_EQ(t.get_element<EnableValve>().m, enableValve.m);
    enableValve.m = "FALSE";
    t.set_element(enableValve);
    EXPECT_EQ(t.get_element<EnableValve>().m, enableValve.m);
}

TEST(Variadic_templatesTest, TestIOstream) {

    std::string input = {
        R"(   ID123.EnableValve := FALSE; 
   ID123.Position1 := 0; 
   ID123.Position2 := 2; 
   ID123.Position3 := 9; 
)"
    };

    std::istringstream ist{ input };

    Variadic_datablock<EnableValve, Position1, Position2, Position3> t;

    ist >> t;

    EXPECT_EQ(t.get_id(), "ID123");
    EXPECT_EQ(t.get_element<EnableValve>().m, "FALSE");
    EXPECT_EQ(t.get_element<Position1>().m, "0");
    EXPECT_EQ(t.get_element<Position2>().m, "2");
    EXPECT_EQ(t.get_element<Position3>().m, "9");


    std::ostringstream ost;

    ost << t;

    EXPECT_EQ(ost.str(), input);
}

Variadic_datablock.h

#pragma once

#include "Read_from_line.h"

#include <tuple>
#include <iostream>
#include <string>
#include <vector>
#include <typeinfo>

template<typename ...T>
class Variadic_datablock
    /*
    Requires elements in T... are unique (not repeat of types)
    */
{
public:
    Variadic_datablock() = default;

    explicit Variadic_datablock(std::string id, T... args)
        :m_id{ std::move(id) },
        m_data{ std::move((std::tuple<T...>(args...))) }
    {
    }

    std::string get_id() const
    {
        return m_id;
    }

protected:
    template<typename Type>
    Type get_element() const
    {
        return std::get<Type>(m_data);
    }

    template<typename Type>
    void set_element(Type a)
    {
        std::get<Type>(m_data) = a;
    }

    std::tuple<T...> get_data() const
    {
        return m_data;
    }

private:
    std::string m_id{};
    std::tuple<T...> m_data{};

    template<typename ...T>
    friend std::ostream& operator<<(std::ostream& os,
        const Variadic_datablock<T...>& obj);

    template<typename ...T>
    friend std::istream& operator>>(std::istream& is,
        Variadic_datablock<T...>& obj);
};

template<class Tuple, std::size_t n>
struct Printer {
    static std::ostream&  print(
        std::ostream& os, const Tuple& t, const std::string& id)
    {
        Printer<Tuple, n - 1>::print(os, t, id);

        auto type_name =
            extract_type_name(typeid(std::get<n - 1>(t)).name());

        os << "   " << id << "." << type_name << " := "
            << std::get<n - 1>(t) << "; " << '\n';
        return os;
    }
};

template<class Tuple>
struct Printer<Tuple, 1> {
    static std::ostream&  print(
        std::ostream& os, const Tuple& t, const std::string& id)
    {
        auto type_name =
            extract_type_name(typeid(std::get<0>(t)).name());

        os << "   " << id << "." << type_name << " := "
            << std::get<0>(t) << "; " << '\n';
        return os;
    }
};

template<class... Args>
std::ostream& print(
    std::ostream& os, const std::tuple<Args...>& t, const std::string& id)
{
    Printer<decltype(t), sizeof...(Args)>::print(os, t, id);
    return os;
}

template<typename ...T>
std::ostream& operator<<(
    std::ostream& os, const Variadic_datablock<T...>& obj)
{
    print(os, obj.m_data, obj.m_id);
    return os;
}

template<class Tuple, std::size_t n>
struct Reader {
    static std::istream&  read(
        std::istream& is, Tuple& t, std::string& last_id)
    {
        Reader<Tuple, n - 1>::read(is, t, last_id);

        auto id = extract_id(is);
        if (!is_expected_id(is, id, last_id)) {
            return is;
        }
        last_id = id;

        auto type_name = extract_type_name(is);
        auto expected_name =
            extract_type_name(typeid(std::get<n - 1>(t)).name());
        if (!is_expected_name(is, type_name, expected_name)) {
            return is;
        }
        dischard_fill(is);

        // can we do this better and extract into type of 
        // actual dispatched tuple?
        auto s = extract_line_value_type(is);

        // prefered: std::get<n - 1>(t) = s but not possible?
        std::get<n - 1>(t).insert(s);

        return is;
    }
};

template<class Tuple>
struct Reader<Tuple, 1> {
    static std::istream&  read(
        std::istream& is, Tuple& t, std::string& last_id)
    {
        auto id = extract_id(is);
        if (!is_expected_id(is, id, last_id)) {
            return is;
        }
        last_id = id;

        auto type_name = extract_type_name(is);
        auto expected_name =
            extract_type_name(typeid(std::get<0>(t)).name());
        if (!is_expected_name(is, type_name, expected_name)) {
            return is;
        }
        dischard_fill(is);

        // can we do this better and extract into the type of 
        // actual dispatched tuple? 
        // typeid(std::get<0>(t)) s = extract_line_value_type(is); ?
        auto s = extract_line_value_type(is);

        // prefered: std::get<0>(t) = s but not possible?
        std::get<0>(t).insert(s);

        return is;
    }
};

template<class... Args>
std::istream& read(
    std::istream& is, std::tuple<Args...>& t, std::string& last_id)
{
    Reader<decltype(t), sizeof...(Args)>::read(is, t, last_id);
    return is;
}

template<typename ...T>
std::istream& operator>>(std::istream& is,
    Variadic_datablock<T...>& obj)
{
    std::tuple<T...> tmp{};
    read(is, tmp, obj.m_id);
    obj.m_data = std::move(tmp);
    return is;
}

Read_from_line.h

#pragma once

#include <iosfwd>
#include <string>

std::string extract_id(std::istream& is);

bool is_expected_id(std::istream& is,
    const std::string& id, const std::string& expected_id);

std::string extract_type_name(std::istream& is);

bool is_expected_name(std::istream& is,
    const std::string_view& name, const std::string_view& expected_name);

void dischard_fill(std::istream& is);

std::string extract_line_value_type(std::istream& is);

std::string erase_whitespace_in_begin(const std::string& s);

std::string extract_type_name(const std::string& typeid_result);

Read_from_line.cpp

#include "Read_from_line.h"

#include <algorithm>
#include <iostream>
#include <sstream>

std::string extract_id(std::istream& is)
{
    std::string id; // id e.g. K101   PI108
    std::getline(is, id, '.');
    id = erase_whitespace_in_begin(id);
    return id;
}

bool is_expected_id(std::istream& is,
    const std::string& id, const std::string& expected_id)
{
    if (expected_id == std::string{}) {
        return true;
    }
    if (id != expected_id) {
        is.setstate(std::ios::failbit);
        return false;
    }
    return true;
}

std::string extract_type_name(std::istream& is)
{
    std::string type_name;      // data e.g. DeviceType
    is >> type_name;
    return type_name;
}

bool is_expected_name(std::istream& is,
    const std::string_view& name, const std::string_view& expected_name)
{
    if (name != expected_name) {
        is.setstate(std::ios::failbit);
        return false;
    }
    return true;
}

void dischard_fill(std::istream& is)
{
    std::string fill;       // fill ":="
    is >> fill;
}

std::string extract_line_value_type(std::istream& is)
{
    std::string value;      // value 10
    std::getline(is, value, ';');
    value = erase_whitespace_in_begin(value);
    return value;
}

std::string erase_whitespace_in_begin(const std::string& s)
{
    std::string ret = s;
    ret.erase(0, ret.find_first_not_of(" \n\r\t"));
    return ret;
}

std::string extract_type_name(const std::string& typeid_result)
{
    std::string ret = typeid_result;

    // case normal name class Test
    if (ret.find('<') == std::string::npos) {
        std::istringstream ist{ ret };
        ist >> ret;
        ist >> ret;
        return ret;
    }

    // Case using Test = Test2<struct TestTag> 
    {
        std::istringstream ist{ ret };
        std::getline(ist, ret, '<');
        std::getline(ist, ret, '>');
    }

    {
        std::istringstream ist2{ ret };
        ist2 >> ret;    // read name such as struct or class
        ist2 >> ret;    // get typenameTag
    }

    ret.erase(ret.find("Tag"));

    if (ret.find("EventMask") != std::string::npos) {
        std::replace(ret.begin(), ret.end(), '_', '.');
    }

    return ret;
}

Fake_types.h

#pragma once

/*
Fake types used to test the template class.
*/

#include <iostream>
#include <string>

template<typename Tag>
struct Faketype {
    std::string m;

    void insert(std::string(s)) {
        m = s;
    }
};

template<typename Tag>
std::istream& operator>>(std::istream& is, Faketype<Tag>& obj)
{
    is >> obj.m;
    return is;
}

template<typename Tag>
std::ostream& operator<<(std::ostream& os, const Faketype<Tag>& obj)
{
    os << obj.m;
    return os;
}

using Position1 = Faketype<struct Position1Tag>;
using Position2 = Faketype<struct Position2Tag>;
using Position3 = Faketype<struct Position3Tag>;

// Only Special case. The file has EventMask.Address1
using EventMask_Address1 = Faketype<struct EventMask_Address1Tag>;

struct EnableValve
{
    std::string m;

    void insert(std::string(s)) {
        m = s;
    }
};

std::istream& operator>>(std::istream& is, EnableValve& obj)
{
    is >> obj.m;
    return is;
}

std::ostream& operator<<(std::ostream& os, const EnableValve& obj)
{
    os << obj.m;
    return os;
}

Please review, sorted by priority:

The Variadic_datablock class: Is it a good design? What can be improved? Is it possible to change std::get<n - 1>(t).insert(s); to std::get<n - 1>(t) = s in the Reader? Please let me know any smells. It's the first time I have used variadic templates. I left some comments in the template which indicate smells, but I don't know how to fix them.
The unit test for the Variadic_datablock class: Are they good test? Easy to understand? Is there something else you would test? Can we ged rid of the protected / public hack?
Read_from_line.h This file provided helper functions for the overloaded Istream Operator of the Variadic_datablock.

Do not review:

Fake_types.h: It is a helper to emulate some datatypes, because the real datatypes I use in my application are more complicated and I wanted to simplyfy to focus on the template

edit: Let me know if you need additional informations to review this.

"But is it possible to obtain a variable name as a string? " Not without reflection unfortunately. Macro hacks are unavoidable right now, and as long as you're not doing anything really crazy, they're fine IMO. — Rakete1111
– Rakete1111, Commented Feb 5, 2019 at 19:13
i found what i wanted was typeid. I used it in the updated code and got rid of the std::vector<std::string> — Sandro4912
– Sandro4912, Commented Feb 6, 2019 at 17:26
It's not clear from your question whether the blocks you read must have a specific format - i.e. specific type names, in specific order, some optional. If not, then how are they used - e.g. how do you know which value to change. — D Drmmr
– D Drmmr, Commented Feb 10, 2019 at 12:21
the blocks habe a specific format. its like i expect a specific format before and then read it in and check each line for the format. — Sandro4912
– Sandro4912, Commented Feb 10, 2019 at 14:31
I'm wondering if it would have been less work to just create pod structs and read/write functions for each seeing as you have to define the structs anyway. I don't know, I just get the feeling you're making things more complicated than they need to be. — Emily L.
– Emily L., Commented Feb 11, 2019 at 10:09

user673679 · Accepted Answer · 2019-02-11 19:10:30Z

Variadic_datablock:

class Variadic_datablock /* Requires elements in T... are unique (not repeat of types) */

We could enforce this requirement with std::enable_if or a static_assert in the class, combined with some template-metaprogramming.
Should an empty parameter pack be allowed? It looks like this would break the print / read functions, so we should probably check for that too.
What's the purpose of the protected member functions? Do we actually need inheritance or could we make everything public except m_id? That would make the class a lot easier to test. Access control in C++ is best used to prevent breaking class-invariants and hide complex internal functionality. The only invariant here seems to be that the ID shouldn't be changed after creation.

The constructor doesn't need to create a temporary tuple. We can move the arguments directly into m_data:

explicit Variadic_datablock(std::string id, T... args):
    m_id{ std::move(id) },
    m_data{ std::move(args)... }
{

}

Printer:

There's some unnecessary duplication. We could definitely abstract this bit into a print_element(os, std::get<n - 1>(t));

auto type_name =
    extract_type_name(typeid(std::get<n - 1>(t)).name());

os << "   " << id << "." << type_name << " := "
    << std::get<n - 1>(t) << "; " << '\n';
return os;

Since the Printer, Reader classes and the print and read functions aren't supposed to be used directly by the user, they could be placed in a detail (or similarly named) namespace.
typeid(x).name() is implementation defined. Several different types may have the same name, and the name can even change between invocations of the same program. In other words, it's not something we should use for serialization. I'd suggest adding a static const std::string data-member to each element class.

Reader:

Same issues as Printer.
It would be reasonable to merge the extract_id and is_expected_id into one function. This means we don't return state that may or may not be valid and then have to pass it into a separate function to check. Sticking with the input operator conventions, we get: std::istream& extract_id(std::istream& is, std::string& id, std::string const& expected_id);. We can use the state of the stream to indicate success / failure, and don't need the extra boolean. e.g.:
```
std::string id;
if (!extract_id(is, id, last_id)) // checks stream fail / bad bits (eof will be caught at next read)
    return is;
```

One test-case with valid input calling the high-level stream operator is not enough to properly confirm the behavior. We need to check the behavior of the individual functions and think about edge cases. For example, for the extract_id function, one might expect the following:

TEST(Test_Reader, IStreamWithValidIDAndDot) { ... }
TEST(Test_Reader, IStreamWithOnlyDot) { ... }
TEST(Test_Reader, IStreamWithIDAndNoDot) { ... }
TEST(Test_Reader, EmptyIStream) { ... }
TEST(Test_Reader, IStreamWithValidIDAndDotAndBadbitSet) { ... }
TEST(Test_Reader, IStreamWithValidIDAndDotAndFailitSet) { ... }
TEST(Test_Reader, IStreamWithValidIDAndDotAndEofbitSet) { ... }

discard, not dischard.
Extra parentheses in FakeType::insert() and EnableValve::insert():
```
void insert(std::string(s)) {
    m = s;
}
```
It's fine to take the argument by value, but we can then move it into place:
```
void insert(std::string s) {
    m = std::move(s);
}
```
We have an operator>> for the FakeTypes, but we aren't using it?
I think we only need to set last_id once for a given datablock.

the purpose of protected is i dont want in all derived classes the option to get and set every Type T. Sometimes only some types will be modified or accesed after read in — Sandro4912
– Sandro4912, Commented Feb 10, 2019 at 14:22
Using perfect forwarding in the constructor now allows users to make Variadic_datablocks that contain references instead of values. I'm not sure this is intended (as Variadic_datablock seems to be used to serialize/deserialize data, and references ba themselves cannot easily be serialized). — hoffmale
– hoffmale, Commented Feb 10, 2019 at 15:26
at the moment i dont use referencws i copy all the value into the datablock by value with std::move. — Sandro4912
– Sandro4912, Commented Feb 10, 2019 at 21:27
@Sandro4912: I was referring to user673679s 4th bullet point in the answer, where he suggests using perfect forwarding in the constructor. (Actually, now that I look at it again, it's not even using forwarding references, but just plain rvalue references, so the lvalue reference point is moot. Still, now the constructor only accepts rvalue references, which doesn't allow for non-movable types... This whole "suggestion" is a mess.) — hoffmale
– hoffmale, Commented Feb 10, 2019 at 21:48
@hoffmale Yeah. I missed that wasn't a template function. Fixed. — user673679
– user673679, Commented Feb 11, 2019 at 8:13

Stack Exchange Network

Variadic Datablocks

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Variadic Datablocks

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions