7
\$\begingroup\$

In reading this question, it occurred to me that it would be helpful to create test files according to the file format specification. To recap, the file format is as follows:

File format

Note that all numeric values are in little-endian format and the checksum is calculated over all of the file except for the checksum field. $$ \begin{array}{l|c|l} \text{name} & \text{length in bytes} & \text{description} \\ \hline \text{file size} & 4 & \text{total number of bytes in file} \\ \text{person count} & 2 & \text{number of Person records in file} \\ \text{Person(s)} & \text{varies} & \text{sequence of Person records} \\ \text{zero padding} & 0..3 & \text{padding to make file size a multiple of 4} \\ \text{checksum} & 4 & \text{checksum of file as 32-bit unsigned values} \end{array} $$

Person record format

$$ \begin{array}{l|c|l} \text{name} & \text{length in bytes} & \text{description} \\ \hline \text{first name size} & 4 & \text{number of chars in first name} \\ \text{first name} & \text{varies} & \text{text of first name (no terminating NUL char)} \\ \text{last name size} & 4 & \text{number of chars in last name} \\ \text{last name} & \text{varies} & \text{text of last name (no terminating NUL char)} \\ \text{flags} & 1 & \text{flag bits: bit 0 = age field present, bit 1 = height field present} \\ \text{age}^* & 1 & \text{age in years. Optional field} \\ \text{height}^* & 1 & \text{height in inches. Optional field} \\ \end{array} $$ An asterisk by a name field indicates that the field is optional.

Questions

The code is mostly plain C++11, but there are a few features I'd like specific comment on. First is the use of the C++20 std::endian. Is there a better way to do this? Second is the use of the static_assert to assure that the iterator dereferences to a Person class. Would enable_if be a better way to handle this? If so, what exactly would the syntax be? Finally, I initially wanted to make this work with a single pass, but the checksum calculation was much cleaner by backing up in the file. Is there an elegant way to do the same checksum calculation without backing up?

maketest.cpp

#include <iostream>
#include <fstream>
#include <string>
#include <iomanip>
#include <array>
#include <sstream>
#include <numeric>

// in C++20 we will have std::endian, but until then, we roll our own
#if 0
#include <type_traits>
#else
namespace std {
enum class endian
{
#ifdef _WIN32
    little = 0,
    big    = 1,
    native = little
#else
    little = __ORDER_LITTLE_ENDIAN__,
    big    = __ORDER_BIG_ENDIAN__,
    native = __BYTE_ORDER__
#endif
};
}
#endif

static_assert(std::endian::native == std::endian::little, "Error: code is only intended for little-endian machines.\n");

class Person {
public:
    Person(const std::string &firstname, const std::string &surname, int age=0, int height=0) :
        firstname{firstname},
        surname{surname},
        age{age},
        height{height}
    {}

    friend std::ostream &operator<<(std::ostream &out, const Person &p) {
        return out << "{ { firstname : " << p.firstname
                    << " }, { surname : " << p.surname
                    << " }, { age : " << p.age
                    << " }, { height : " << p.height
                    << " }";
    }
    std::size_t length() const {
        return 9 + firstname.size() + surname.size() + (age ? 1 : 0) + (height ? 1 : 0);
    }
    std::size_t write(std::ostream &out) const {
        auto begin{out.tellp()};
        std::uint8_t flags{static_cast<std::uint8_t>((height ? 2 : 0) | (age ? 1 : 0))};
        std::uint32_t firstlen{static_cast<uint32_t>(firstname.size())};
        std::uint32_t lastlen{static_cast<uint32_t>(surname.size())};
        out.write(reinterpret_cast<const char *>(&firstlen), sizeof firstlen);
        out << firstname;
        out.write(reinterpret_cast<const char *>(&lastlen), sizeof lastlen);
        out << surname;
        out.write(reinterpret_cast<const char *>(&flags), sizeof flags);
        if (age) {
            out.write(reinterpret_cast<const char *>(&age), 1);
        }
        if (height) {
            out.write(reinterpret_cast<const char *>(&height), 1);
        }
        return out.tellp() - begin;;
    }

private:
    std::string firstname;
    std::string surname;
    int age;
    int height;
};


template<class InputIt>
std::ostream &write(InputIt first, InputIt last, std::iostream &out)
{
    static_assert(std::is_convertible<decltype(*first), const Person&>::value, "Error: iterator to write() must dereference to Person class");
    std::uint32_t total_len{10 + std::accumulate(first, last, 0u, [](unsigned t, const Person &p){ return t + p.length(); })};
    std::size_t pad_len{4 - (total_len % 4)};
    if (pad_len == 4) {
        pad_len = 0;
    }
    total_len += pad_len;
    out.write(reinterpret_cast<const char *>(&total_len), sizeof total_len);
    std::uint16_t count{static_cast<uint16_t>(last - first)};
    out.write(reinterpret_cast<const char *>(&count), sizeof count);
    for (auto &p{first}; p != last; ++p) {
        p->write(out);
    }
    unsigned long pad{0};
    out.write(reinterpret_cast<const char *>(&pad), pad_len);
    out.seekg(0);
    std::uint32_t cksum{0};
    for (std::size_t i{total_len / 4 - 1}; i; --i) {
        std::uint32_t piece;
        out.read(reinterpret_cast<char *>(&piece), sizeof piece);
        cksum += piece;
    }
    out.write(reinterpret_cast<const char *>(&cksum), sizeof cksum);
    return out;
}

int main(int argc, char *argv[]) {
    const std::array<Person, 6> people {{
        { "Abigail", "Adams", 6, 0 },
        { "Bob", "Barker", 40, 72 },
        { "Charles", "Cook", 0, 55 },
        { "Deborah", "Dawson", 0, 0 },
        { "Edward", "Electron", 45, 70 },
        { "Freddie", "Freeloader", 0, 0 },
    }};
    if (argc != 2) {
        std::cout << "Usage: maketest filename\n";
        return 0;
    }
    std::fstream out{argv[1], std::ios_base::in|std::ios_base::out|std::ios_base::trunc};
    if (!out) {
        std::cerr << "Error opening file \"" << argv[1] << "\"\n";
        return 1;
    }
    write(people.begin(), people.end(), out);
}

Result

I used this command to compile, run, and examine the results:

make maketest && maketest test.in && xxd test.in

The resulting file, as displayed by xxd is this:

00000000: 9400 0000 0600 0700 0000 4162 6967 6169  ..........Abigai
00000010: 6c05 0000 0041 6461 6d73 0106 0300 0000  l....Adams......
00000020: 426f 6206 0000 0042 6172 6b65 7203 2848  Bob....Barker.(H
00000030: 0700 0000 4368 6172 6c65 7304 0000 0043  ....Charles....C
00000040: 6f6f 6b02 3707 0000 0044 6562 6f72 6168  ook.7....Deborah
00000050: 0600 0000 4461 7773 6f6e 0006 0000 0045  ....Dawson.....E
00000060: 6477 6172 6408 0000 0045 6c65 6374 726f  dward....Electro
00000070: 6e03 2d46 0700 0000 4672 6564 6469 650a  n.-F....Freddie.
00000080: 0000 0046 7265 656c 6f61 6465 7200 0000  ...Freeloader...
00000090: 15b1 8582                                ....

Note that this checksum does not match the one calculated by the code in the linked question. It calculates 0x8285b015 instead, which I believe is an error caused by that code's failure to account for inter-byte carry.

\$\endgroup\$
  • \$\begingroup\$ enable_if is primarily for situations where you want to enable different sections of code statically. It would be useful if you wanted to add support to run on a big-endian system. Barring that, your use of static_assert seems entirely reasonable to me. \$\endgroup\$ – Jerry Coffin Feb 21 '18 at 0:25
  • \$\begingroup\$ I had some doubts about why you used 4 bytes for the name length, but now I realize there are names with 512+ characters long. Had to google for it. I'm not sure though if all of it is last name. \$\endgroup\$ – Incomputable Feb 22 '18 at 10:09
  • \$\begingroup\$ @Incomputable yes, if I were designing the file format, I’d do it differently. Not least, I’d use a CRC instead of a checksum. \$\endgroup\$ – Edward Feb 22 '18 at 13:45
  • \$\begingroup\$ @Edward, then are you interested in implementation that uses start and end blocks? \$\endgroup\$ – Incomputable Feb 22 '18 at 15:41
  • \$\begingroup\$ @Incomputable, if it's a better way to solve this problem and conforms to the given file specification, then probably so. I'm always interested in learning better/smarter/faster ways of doing things. \$\endgroup\$ – Edward Feb 22 '18 at 15:44
1
\$\begingroup\$

The assert seems a little bit off, it should be:

static_assert(std::is_convertible<typename std::iterator_traits<InputIterator>::reference, const Person&>::value, "Error: iterator to write() must dereference to Person class");

Why? In [input.iterators], N3797 (page 832), the table says that *it must be convertible to T. I'm too tired to pull out conversion rules from the top of my head, but I believe the way above guarantees that at least it will discard incorrect ones and not run into one of those dangling reference realms. This would be a great language lawyer question.

When to use enable_if (e.g. SFINAE)?

As the title says, std::enable_if shall be used only in SFINAE context. If template instantiation doesn't pass enable check, and doesn't have anything to fallback into, it's gonna be just more cryptic static_assert. SFINAE is generally harder to get right, and tends to have small bugs that require very in-depth understanding of templates to solve.

\$\endgroup\$

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.