2
\$\begingroup\$

I'm computing the crc32 of a given file using boost:

uint32_t crc32(const std::string &fp){
    boost::crc_32_type result;
    std::ifstream is{fp, std::ios::in | std::ios::binary};
    size_t file_size = is.tellg();
    is.seekg(0, std::ios::beg);
    std::vector<char> buf(file_size);
    is.read(&buf[0], file_size);

    result.process_bytes(&buf[0], file_size);
    return result.checksum();
}

I believe this code is correct, but it seems unecssarily verbose, and does 2 passes over the data.

Ideally I'd like to write something like:

uint32_t crc32(const std::string &fp){
    boost::crc_32_type result;
    std::ifstream is{fp, std::ios::in | std::ios::binary};
    size_t file_size = is.tellg();
    result.process_block(std::istreambuf_iterator<char>{is}, {}); //invalid
    return result.checksum();
}

But I'm not sure there's a valid way to convert the iterator to char *.

The final alternative I tried was:

for(uint8_t b : std::istreambuf_iterator<char>{is}){ //invalid
    result.process_byte(b);
}
return result.checksum();

But I'm unclear how to get a range from a istreambuf_iterator.

\$\endgroup\$
  • \$\begingroup\$ The range based for using std::begin() and std::end() to get the range it will iterate over. So you just need to write a wrapper class that will respond to these function calls: gist.github.com/Loki-Astari/ac8763f0032ad27819c0d687a6732810 \$\endgroup\$ – Martin York Jun 30 '16 at 15:11
  • \$\begingroup\$ @LokiAstari thanks, that makes sense. Since I'm only using this range in one place I might be more inclined to just use a iterator for(auto i = std::istreambuf_iterator<char>{is}; i != {}; i++) rather than add a wrapper class (even though the range approach feels neater). \$\endgroup\$ – cloakedlearning Jun 30 '16 at 15:21
4
\$\begingroup\$

You seem to be missing a seek to the end.

This will always be zero.

    std::ifstream is{fp, std::ios::in | std::ios::binary};
    size_t file_size = is.tellg();

You need to add a seek to end:

    std::ifstream is{fp, std::ios::in | std::ios::binary};
    is.seekg(0, std::ios::end);
    size_t file_size = is.tellg();

There are other functions that query the file system to get the size. But You should note this does NOT do a scan of the file it just moves the descriptor to the end. So its not like reading the file.

You could just wrap it.

std::size_t fileSize(std::ifstream& file)
{
    std::streampos current = file.tellg();
    file.seek(0, std::ios::end);
    std::size_t result = file.tellg();
    file.seek(current, std::ios::beg);
    return result;
}

Now your function becomes:

std::uint32_t crc32(const std::string &fp)
{
    std::ifstream is{fp, std::ios::in | std::ios::binary};
    std::size_t file_size = fileSize(is);

    std::vector<char> buf(file_size);
    is.read(&buf[0], file_size);

    boost::crc_32_type result;
    result.process_bytes(&buf[0], file_size);

    return result.checksum();
}

The problem with this is that some files are huge. You don't want to read the whole thing into memory like that. So you could define a reasonable size buffer and read chunks at a time.

 std::vector<char>   buffer(4096);

 while(is.read(&buffer[0], buffer.size()))
 {
     std::size_t count = is.gcount();
     result.process_bytes(&buffer[0], count);
 }

Or you could use a memory mapped file:

Its been a while since I did this. Here are some instructions.

https://www.safaribooksonline.com/library/view/linux-system-programming/0596009585/ch04s03.html

\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments. Re. seeking to the end, could I i just use std::ios::ate or would be insufficient? \$\endgroup\$ – cloakedlearning Jun 30 '16 at 15:40
  • \$\begingroup\$ @cloakedlearning: That is something you can easily test. \$\endgroup\$ – Martin York Jun 30 '16 at 15:44
  • \$\begingroup\$ Sorry, i mis-worded my question. I believe it would be correct - but would it be in poor style? \$\endgroup\$ – cloakedlearning Jun 30 '16 at 15:45
  • \$\begingroup\$ The loop approach is nice, but it is relevant to note that you have to check for the number of bytes read at each iteration. It is unlikely that the last read will be exactly 4096 bytes. \$\endgroup\$ – glampert Jun 30 '16 at 18:09
  • \$\begingroup\$ @glampert: And that is why we have code review. \$\endgroup\$ – Martin York Jun 30 '16 at 19:21
4
\$\begingroup\$

I've got little to add to Martin Yorks great answer.

I also think the loop approach is the better one, since it limits the amout of memory used for bigger files.

However, I found a little mistake in the answer: Since a while loop is used and the read method of the stream returns a reference to the stream, the condition evaluates to false if the end of the file has been reached. Thus, the last block ist not used in the CRC calculation.

I therefore propose the following solution which is also closer to the example provided with the library:

boost::crc_32_type crc;
std::vector<char> buffer(4096);
std::ifstream stream(fp, std::ios::in|std::ios::binary);
if(!stream) throw std::ifstream::failure("Could not open file for CRC calculation: " + fp);
do {
    stream.read(&buffer[0], buffer.size());
    size_t byte_cnt = static_cast<size_t>(stream.gcount());
    crc.process_bytes(&buffer[0], byte_cnt);
} while(stream);
return crc.checksum();

As you can see I also added an exception in case of file opening errors which should of course be caught somewhere.

I checked the result against the CRC functionality in 7-Zip, and now it finally matches :)

\$\endgroup\$
2
\$\begingroup\$

Failing to test whether istream::read() succeeds means that any read failure will lead to computing an incorrect CRC.

There's no need to read the full file into memory, and no need to measure its size in advance - we can read chunks at a time using a sensible size buffer like this:

#include <boost/crc.hpp>

#include <istream>

uint32_t crc32(std::istream &is)
{
    char buf[4096];
    boost::crc_32_type result;

    do {
        is.read(buf, sizeof buf);
        result.process_bytes(buf, is.gcount());
    } while (is);

    if (is.eof()) {
        return result.checksum();
    } else {
        throw std::runtime_error("File read failed");
    }
}

Notice that we're now using an opened input stream object instead of passing a file name. We could be even more flexible, passing a pair of iterators - but then we're at a point where the CRC engine just needs a standard algorithm to feed the range into its operator():

#include <boost/crc.hpp>

#include <algorithm>
#include <functional>
#include <istream>
#include <iterator>

uint32_t crc32(std::istream &is)
{
    std::istreambuf_iterator in{is};
    decltype(in) end{};
    boost::crc_32_type result;
    std::for_each(in, end, std::ref(result));

    if (is.peek() == EOF) {
        return result.checksum();
    } else {
        throw std::runtime_error("File read failed");
    }
}
\$\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.