Chunking strings to binary block-based output

Question

This is a follow up to Chunking strings to binary output

I found the reviews quite helpful in pinpointing what to fix with my code. Here's the result of that update. The format of the output is the same as documented there, but to briefly recap:

The stream format consists of blocks, each 256 bytes long. Each block begins with a fixed 4-byte block identifier and ends with a 4-byte checksum. Everything between them is data.

The data is in the form of counted strings. A counted string is a one byte unsigned integer \$n\$ followed by that many bytes of data. A counted string may or may not be NUL character terminated. All counted strings are 0 to 255 bytes long with a length of 0 signifying a blank line.

The test program reads in a text file, the name of which is passed on the command line. Each line is then converted to a counted string and output as Blocks to the binary output file, also passed as a command line argument.

Questions

I think the separation is much better, but I'd really rather have hidden the entire definition of Block within the Chunkster.cpp file. There didn't seem to be an elegant way to do that. I tried a version that used the pimpl idiom, but it seemed ugly to me.

Chunkster.h

#ifndef CHUNKSTER_H
#define CHUNKSTER_H
#include <iostream>
#include <fstream>
#include <string_view>
#include <cstdint>
#include <algorithm>
#include <numeric>
#include <array>

class Block {
public:
    //! each block is this many bytes
    static constexpr std::size_t mysize{0x100};
    //! read a block 
    friend std::istream& operator>>(std::istream& in, Block& blk);
    //! write a block 
    friend std::ostream& operator<<(std::ostream& out, const Block& blk);
    //! append passed data to this Block and return bytes actually written
    std::size_t append(const void *mydata, std::size_t len);
    //! reset the Block by clearing data
    void reset();
    //! fix the Block's checksum to correct value
    uint32_t fixSum() { return checksum = sumcalc(); }
    //! returns true if checksum is correct
    bool isGood() const { return sumcalc() == checksum; }
    //! returns true if no more bytes can be stored in Block
    bool isFull() const { return remaining == 0; }
private:
    //! calculates the checksum of the block
    uint32_t sumcalc() const;
    //! the magic value used for each block
    static constexpr uint32_t magic{0xfecaadbe};
    //! the id value (should always equal magic)
    uint32_t id = magic;
    //! checksum of Block
    uint32_t checksum = magic;
    //! size of data portion of Block in bytes
    static constexpr std::size_t datasize{mysize - sizeof(id) - sizeof(checksum)};
    //! the data portion of the Block
    std::array<char, datasize> data;
    //! number of unused bytes remaining in the data section of this Block
    std::size_t remaining{datasize};
    //! pointer to the next unused byte in the data section of this Block
    char *curr = &data[0];
};

class Chunkster {
public:
    //! constructor for writing chunks
    Chunkster(const char *filename, std::ios_base::openmode mode = std::ios_base::out);
    //! write passed string to chunkified output
    bool write(std::string_view str);
    //! destructor
    virtual ~Chunkster();
private:
    //! flush the last written chunk if the current Block is full
    void flushIfFull(); 
    //! unconditionally flush output
    void flush();

    //! output file
    std::ofstream out;
    //! current Block used for output
    Block current;
};

#endif // CHUNKSTER_H

Chunkster.cpp

#include "Chunkster.h"
// will have this in C++20, but not yet implemented
#define HAVE_SPAN 0
#if HAVE_SPAN
#include <span>
#endif

std::istream& operator>>(std::istream& in, Block& blk) {
    blk.data.fill(0);
    in.read(blk.data.begin(), sizeof(blk.data));
    in.read(reinterpret_cast<char *>(&blk.checksum), sizeof(blk.checksum));
    return in;
}

std::ostream& operator<<(std::ostream& out, const Block& blk) {
    out.write(reinterpret_cast<const char *>(&blk.id), sizeof(blk.id));
    out.write(blk.data.begin(), sizeof(blk.data));
    out.write(reinterpret_cast<const char *>(&blk.checksum), sizeof(blk.checksum));
    return out;
}

// append passed data to this Block and return bytes actually written
std::size_t Block::append(const void *mydata, std::size_t len) {
    len = std::min(len, remaining);
    auto ptr{reinterpret_cast<const char *>(mydata)};
    std::copy(ptr, ptr+len, curr);
    remaining -= len;
    curr += len;
    return len;
}

void Block::reset() { 
    data.fill(0);
    remaining = datasize;
    curr = &data[0];
}

uint32_t Block::sumcalc() const {
#if HAVE_SPAN
    std::span as_u32{reinterpret_cast<std::uint32_t*>(data.begin()),
                     reinterpret_cast<std::uint32_t*>(data.end())};
    return std::accumulate(as_u32.begin(), as_u32.end(), std::uint32_t{});
#else
    return std::accumulate(
        reinterpret_cast<const std::uint32_t*>(data.begin()),
        reinterpret_cast<const std::uint32_t*>(data.end()),
        id);
#endif
}

Chunkster::Chunkster(const char *filename, std::ios_base::openmode mode) :
    out{filename, mode}
{}

bool Chunkster::write(std::string_view str) {
    if (str.length() < 256) {
        uint8_t n = str.length();
        current.append(&n, 1);
        flushIfFull();
        std::size_t index{0};
        while(n) {
            auto written = current.append(&str[index], n);
            //current.dump(std::cout);
            n -= written;
            index += written;
            flushIfFull();
        }
    }
    return out.good();
}

Chunkster::~Chunkster() {
    flush();
}

void Chunkster::flushIfFull() {
    if (current.isFull()) {
        flush();
        current.reset();
    }
}

void Chunkster::flush() {
    current.fixSum();
    out << current;
}

main.cpp

#include "Chunkster.h"
#include <string>
#include <iostream>
#include <fstream>

int main(int argc, char *argv[]) {
    std::string line;
    if (argc != 3) {
        std::cerr << "Usage: encode infile outfile\n";
        return 1;
    }
    std::ifstream in(argv[1]);
    Chunkster out{argv[2], std::ios_base::binary};
    while (std::getline(in, line)) {
        out.write(line);
    }
}

Answer 1

Pass a std::ostream to Chunkster() instead of a filename

The goal of your class Chunkster is to convert the format of one stream to another, it shouldn't have to open and close files. Just pass a std::ostream to the constructor instead of a filename and open mode. This makes your class simpler and at the same time more flexible, because now you could have it write to a std::stringstream or any other type that inherits from std::ostream.

Be more thorough handling errors

There is some attempt at error handling in your code, but it falls short. For example, write() returns a boolean to indicate whether the output stream is still good, but flush() and flushIfFull() do no such thing. Either have all functions that potentially do I/O return something indicating success, or add a separate function that can be used to check the current error state, perhaps a bool Chunkster::good().

Another issue is that write() returns success if you give it a string longer than 255 characters. You should return an error in this case.

Move class Block into class Chunkster

Since a Block is only a utility class for Chunkster, and not meant to be used by anything else but a Chunkster, it is better to move this into Chunkster itself, so it doesn't pollute the global namespace. So it would look like:

class Chunkster {
    ...

    private:
    class Block {
        ...
    } current;
};

If you really want to keep them separate, I recommend you put both Block and Chunkster in their own namespace.

Avoid clearing data unnecessarily

Every time you flush a block, you the call reset(), which fills the block with zero bytes. However, in normal use, you would fill the whole block with new strings, so all zeroes are overwritten. It might be more efficient to just zero the unused bytes of a block right before calculating the checksum.

Avoid writing multiple implementations of a function for different versions of C++

In sumcalc(), you have two implementations, one for C++20 where you use spans, and one for earlier versions of C++, and you use #ifdefs to select which version to use at compile time. I would avoid doing this, because there is absolutely no difference in performance here, and the non-span version works just as well on C++20.

In general, set a minimum C++ version for your project, and code against that. The only time you should use #ifdefs to provide alternative implementations is when you want to provide a different interface for your classes, so it is easier to use with newer versions of C++. For example, if your minimum version was C++11, then it would make sense to provide a version of write() that takes a const reference to a std::string, and then also provide a write() function that takes a std::string_view, but only compile that one conditionally.

Code duplication can lead to errors. For example, depending on whether or not you have spans, you use a different initial value for the accumulation: std::uint32_t{} for the span version, id for the other version.