6
\$\begingroup\$

I have implemented a function to produce a 32-bit checksum of a file using the following method: checksum = word_1 + word_2 + ... + word_n, where word_i is the 32-bit words the file consists of.

Here are several questions I'm very interested about:

  1. Is the way I read file word by word correct or there is a better way? (I aim not to read the whole file at once because it can be very large.)
  2. Are there any problems with chosen data types such as uint32_t, unsigned and so on?
  3. Have I got the right way to handle a file that isn't N*4 bytes in size? For example, for 7 bytes file I'm just setting 0-8 bits into 0 to avoid using an accidental value. Or I should set 24-31 bits into 0?

Here is the code I have so far:

void execute(std::ifstream& file) 
{
    const size_t WORD_SIZE = sizeof(uint32_t);

    file.seekg(0, ios::end);
    auto sizeInBytes = file.tellg();

    file.seekg(0);
    uint32_t checksum = 0U; // ???
    if(auto sizeInEntireWords = sizeInBytes / WORD_SIZE)
    {
        for(int i = 0; i < sizeInEntireWords; i++)
        {
            uint32_t word;
            file.read(reinterpret_cast<char*>(&word), WORD_SIZE); // ???
            checksum += word;
        }
    }

    if(auto additionalSizeInBytes = sizeInBytes % WORD_SIZE)
    {
        uint32_t word;
        file.read(reinterpret_cast<char*>(&word), WORD_SIZE);
        word &= (~0U << (WORD_SIZE - additionalSizeInBytes * 8)); // ???
        checksum += word;
    }

    cout << checksum << endl;
}
\$\endgroup\$
  • \$\begingroup\$ Are you sure assigning inside an if is not a bug in if(auto sizeInEntireWords = sizeInBytes / WORD_SIZE)? \$\endgroup\$ – Caridorc Sep 17 '15 at 19:12
  • \$\begingroup\$ @Caridorc, yes, it's possible. I've read about it in Stroustrup's book. More over it's cool on account of the variable (sizeInEntireWords) being more localized: it's only available in -f-{} and its else-{}. \$\endgroup\$ – anton.vodostoev Sep 17 '15 at 20:25
  • \$\begingroup\$ If assignment fails the condition is false right? \$\endgroup\$ – Caridorc Sep 17 '15 at 20:26
  • \$\begingroup\$ sizeInEntireWords is created here and, if its value equals 0, it's converted into false. Otherwise it's converted into true. \$\endgroup\$ – anton.vodostoev Sep 17 '15 at 20:29
  • \$\begingroup\$ if(size_t i = container.size()) { cout << "We're here if container is NOT empty and can use i that is != 0"; } else { cout << "We're here if container is empty and can use i that is == 0"; } // but i is not available here \$\endgroup\$ – anton.vodostoev Sep 17 '15 at 20:33
5
\$\begingroup\$

First of all, your checksum function should probably be named checksum and actually return the checksum, rather than simply printing it.

Now, you're not taking advantage of what the std::basic_ifstream API actually gives you. First, read() returns a basic_istream&, which is convertible to bool. That bool tells you if the complete read succeeded or not. So all you have to do is:

uint32_t checksum(std::ifstream& file) 
{
    uint32_t sum = 0;

    uint32_t word = 0;
    while (file.read(reinterpret_cast<char*>(&word), sizeof(word))) {
        sum += word;
    }

    // ??

Now, when read() fails, that means our file has run out (assuming we had a valid file to begin with). But, we have other information. There is also gcount() which

Returns the number of characters extracted by the last unformatted input operation.

If the read partially succeeded, we can mask off the other bytes and add the remainder. Thus, the full solution might be:

uint32_t checksum(std::ifstream& file) 
{
    uint32_t sum = 0;

    uint32_t word = 0;
    while (file.read(reinterpret_cast<char*>(&word), sizeof(word))) {
        sum += word;
    }

    if (file.gcount()) {
        word &= (~0U >> ((sizeof(uint32_t) - file.gcount()) * 8));
        sum += word;
    }

    return sum;
}

Doing the masking looks kind of terrible though, so instead we could simply zero out word every time so we can just add the result:

uint32_t checksum(std::ifstream& file) 
{
    uint32_t sum = 0;

    uint32_t word = 0;
    while (file.read(reinterpret_cast<char*>(&word), sizeof(word))) {
        sum += word;
        word = 0;
    }

    sum += word; // add the last word, could be 0
                 // if the file size is divisible by 4

    return sum;
}
\$\endgroup\$
  • \$\begingroup\$ To be honest, I do not completely understand if the last example correctly handles last 1 or 2 or 3 bytes, for example. They are read into 4-byte word. So there might be accident value? Let file be of 7 bytes. The loop will be executed 2 times, right? Will the second word fully determined in the case? \$\endgroup\$ – anton.vodostoev Sep 17 '15 at 20:41
  • \$\begingroup\$ Thanks! And one more question please: (assuming the file is of 7 bytes) the second 32-bit word will consist of 3 bytes read from file and what is with 1 more byte? Will it be 0000 0000? So the last word will be 0000 0000 **** **** **** **** **** ****, where *'s are 3 read bytes representation? I'm scared of some rubbish in the last word:) It's important to be strongly determined in case of file size not devisible by 4. \$\endgroup\$ – anton.vodostoev Sep 17 '15 at 21:03
  • \$\begingroup\$ Yes, It seems that if a word is set into 0 before new reading, while we read 3 last bytes, we rewrite younger 3 bytes of the word. The oldest byte is still consists of 0s? \$\endgroup\$ – anton.vodostoev Sep 17 '15 at 21:16
4
\$\begingroup\$
  • Portability

    The same file checksums differently depending on platform endianess. Fix it by word = htonl(word); after reading.

  • Algorithm

    I hope you understand that such checksum is useless to say the least.

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

I see a number of things that may help you improve your code.

Use better names

The name execute is too generic and not at all suggestive of the purpose of the function. The obvious choice would be checksum of course. It seems like a small point, but choosing good names makes a very big difference in how easy (or not!) it is to read, understand, and modify the code.

Return something useful

Instead of declaring this as a void function, it would make much more sense to declare it as uint32_t and to return the actual checksum value.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. I don't know that you've actually done that, (you might have used the slightly more enlightened using std::cout;) but it's an alarmingly common thing for new C++ programmers to do.

Be aware of endianness problems

As pointed out by @vnp, this will calculate different results on machines with a different endianness. The simple way to address that would be to read byte (uint8_t) at a time and create your own uint32_t in portable fashion.

Consider a better algorithm

For only very slightly more complexity, you can get a much better result by using either a CRC or a cryptographic hash. Either would provide detection of, say, swapped words, which this algorithm cannot.

Putting it together

uint32_t checksum(std::ifstream& file) 
{
    uint32_t checksum = 0;
    unsigned shift = 0;
    for (uint32_t ch = file.get(); file; ch = file.get()) {
        checksum += (ch << shift);
        shift += 8;
        if (shift == 32) {
            shift = 0;
        }
    }
    return checksum;
}

This calculates the sum as little-endian on any machine and returns that value in the native representation of the machine on which it runs.

\$\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.