8
\$\begingroup\$

I wrote a program to insert a checksum into a Game Boy cartridge. Here is a spec of the cartridge header, but I'll include the relevant information here, as well.

The header checksum is defined as the sum of the bitwise-NOT of the 52nd through the 76th bytes of the file, inclusive.

The global checksum is defined as the sum of all the bytes in the file except for the bytes of the global checksum itself.

This program requires placeholder zero bytes for these checksums. If the placeholder bytes are not zero, the program errors.

Here's the program:

#include <errno.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>

// Insert various checksums into a Game Boy cartridge file.
// Gives an error if the checksums are present prior to checksumming.

#define HEADER_DATA \
/* nop; jmp $150 */ \
"\x00\xc3\x50\x01" \
/* Nintendo logo */ \
"\xce\xed\x66\x66\xcc\x0d\x00\x0b\x03\x73\x00\x83\x00\x0c\x00\x0d" \
"\x00\x08\x11\x1f\x88\x89\x00\x0e\xdc\xcc\x6e\xe6\xdd\xdd\xd9\x99" \
"\xbb\xbb\x67\x63\x6e\x0e\xec\xcc\xdd\xdc\x99\x9f\xbb\xb9\x33\x3e"

enum cartridge_header_section_sizes
{
    // oversimplification, but the extra data is not needed
    ENTRY = 0x0,
    MISC = ENTRY + 0x34,
    HEADER_CHECKSUM = MISC + 0x19,
    GLOBAL_CHECKSUM = HEADER_CHECKSUM + 0x01,
};

// checksum functions save and restore file position
static unsigned char header_checksum(FILE *const fp, size_t nbytes)
{
    fpos_t pos;
    unsigned char result;

    if(fgetpos(fp, &pos) == -1)
        return -1;

    for( result = 0; nbytes--; )
    {
        int c = fgetc(fp);

        if(c == -1)
            return -1;

        result += (unsigned char)~(unsigned char)c;
    }

    if(fsetpos(fp, &pos) == -1)
        return -1;

    return result;
}

static unsigned short global_checksum(FILE *const fp, size_t nbytes)
{
    fpos_t pos;
    unsigned short result;

    if(fgetpos(fp, &pos) == -1)
        return -1;

    for( result = 0; nbytes--; )
    {
        int c = fgetc(fp);

        if(c == -1)
            return -1;

        result += (unsigned char)c;
    }

    if(fsetpos(fp, &pos) == -1)
        return -1;

    return result;
}

int main(const int argc, const char *const *const argv)
{
    if(argc != 2)
    {
        printf("Usage: %s gb-file\n", argv[0]);
        goto fail;
    }

    FILE *const fp = fopen(argv[1], "r+b");
    if(fp == NULL)
    {
        perror(argv[1]);
        goto fail;
    }

    // -1 for the trailing null byte
    unsigned char header[sizeof(HEADER_DATA) - 1];
    if(fread(header, 1, sizeof(header), fp) != sizeof(header))
    {
        fputs("Short file: header read failed\n", stderr);
        goto fail;
    }

    if(memcmp(header, HEADER_DATA, sizeof(header)) != 0)
    {
        fputs("Invalid header!\n"
              "Make sure that your header contains nop; jp $150 "
              "and the official Nintendo logo before running again.\n",
              stderr);
        goto fail;
    }

    errno = 0;
    unsigned char hchk = header_checksum(fp, HEADER_CHECKSUM - MISC);
    if(hchk == (unsigned char)-1 && errno)
    {
        perror("header checksum");
        goto fail;
    }
#ifndef NDEBUG
    printf("%hx\n", hchk);
#endif
    if(fseek(fp, HEADER_CHECKSUM, SEEK_SET) == -1)
    {
        perror("fseek");
        goto fail;
    }
    // sanity checking
    if(fgetc(fp))
    {
        fputs("Header checksum already in place!\n"
              "Did you run the program twice?\n", stderr);
        goto fail;
    }
    // rewind to previous position if successful
    if(fseek(fp, -1, SEEK_CUR) == -1)
    {
        perror("fseek");
        goto fail;
    }
    if(fwrite(&hchk, 1, sizeof(hchk), fp) != sizeof(hchk))
    {
        fputs("Insertion of header checksum failed.\n", stderr);
        goto fail;
    }

    if(fseek(fp, 0, SEEK_END) == -1)
    {
        perror("fseek");
        goto fail;
    }

    long size = ftell(fp);
    if(size == -1)
    {
        perror("ftell");
        goto fail;
    }

    if(fseek(fp, 0, SEEK_SET) == -1)
    {
        perror("fseek");
        goto fail;
    }

    // eh, why not?
    errno = 0;
    unsigned short gchk = global_checksum(fp, (size_t)size);
    if(gchk == (unsigned short)-1 && errno)
    {
        perror("global checksum");
        goto fail;
    }
#ifndef NDEBUG
    printf("%hx\n", gchk);
#endif
    unsigned char gchk_arr[2] = { gchk >> 8, gchk & 0xff };

    if(fseek(fp, GLOBAL_CHECKSUM, SEEK_SET) == -1)
    {
        perror("fseek");
        goto fail;
    }
    // more sanity checking
    int c1 = fgetc(fp);
    int c2 = fgetc(fp);
    if(c1 || c2)
    {
        fputs("Global checksum already in place!\n"
              "Did you run the program twice?\n", stderr);
        goto fail;
    }
    if(fseek(fp, -2, SEEK_CUR) == -1)
    {
        perror("fseek");
        goto fail;
    }
    if(fwrite(gchk_arr, 1, sizeof(gchk_arr), fp) != sizeof(gchk_arr))
    {
        perror("fwrite");
        goto fail;
    }

    fflush(fp);
    fclose(fp);

    return EXIT_SUCCESS;
fail:
    if(fp)
        fclose(fp);
    return EXIT_FAILURE;
}

What I'm looking for:

  • Is there any way to simplify this? It seems that I read, seek, and re-read the file many times, and I repeat code a lot.
  • Are my variable, structure, and function names self-explanatory? Do the comments help improve the readability of the program?
  • Are there any edge-cases that I might've missed when testing this?
  • Any other general advice.
\$\endgroup\$
6
  • \$\begingroup\$ Why C89? Is this being compiled for a specific platform that has a terrible compiler? \$\endgroup\$
    – Reinderien
    Mar 28 '20 at 16:09
  • 1
    \$\begingroup\$ Distant back-compatibility comes at a cost (not naming any names, but Intel). There's some syntactical sugar you're missing out on that helps with maintainability. If you have a specific application in mind that requires C89, fine; but it doesn't seem like that's the case. \$\endgroup\$
    – Reinderien
    Mar 28 '20 at 16:14
  • \$\begingroup\$ @Reinderien Yeah. I deleted my comment. I also began to think about why I am only using C89. I think it's because of Windows. \$\endgroup\$
    – S.S. Anne
    Mar 28 '20 at 16:16
  • 1
    \$\begingroup\$ @Reinderien C99 now. Sweet syntatic sugar. \$\endgroup\$
    – S.S. Anne
    Mar 28 '20 at 16:19
  • \$\begingroup\$ S.S. Anne, "only using C89. I think it's because of Windows" --> Widows does not restrict C to C89. I am running C11 on a Windows machine. The restriction you may see comes from Visual Studio, not the OS. \$\endgroup\$ Mar 28 '20 at 23:34
4
\$\begingroup\$

Wrong compare

fgetc() returns an int with the value of EOF or something in the unsigned char range. Although EOF is commonly -1, it is not specified as so.

    int c = fgetc(fp);
    // if(c == -1)
    if(c == EOF)

Useless cast

The 2nd cast is not needed as c is in the unsigned char range so there is no value change. Before the ~ is applied, the (unsigned char)c is converted to int. No value nor type change --> cast not needed.

// result += (unsigned char)~(unsigned char)c;
result += (unsigned char)~c;

The first cast not needed either there. result += (unsigned char)~c; is same as result = result + (unsigned char)~c;. Both result and (unsigned char)~c are promoted to int before the addition. The cast in (unsigned char)~c does not affect the end result.

A cast is useful just before the assignment to quiet int to unsigned char warnings. Suggest the following:

// result += (unsigned char)~c;
result = (unsigned char) (result + ~c);

Clarity

Alternative that, IMO, is more clear.

// hchk == (unsigned char)-1
hchk == UCHAR_MAX

Strange format specifier choice

Unclear why code uses "%hx". Usually that is for unsigned short.

 unsigned char hchk;
 ...
// printf("%hx\n", hchk);
printf("%hhx\n", hchk);
// or
printf("%x\n", hchk);  // the hh is not truly needed, but it does add some clarity

Wrong error test

"on failure, the fgetpos function returns nonzero"

// if(fgetpos(fp, &pos) == -1)
if(fgetpos(fp, &pos))

Note: good use of fsetpos(), fgetpos(), versus fseek(), ftell(). Unclear why code uses fseek() elsewhere.

Performance

header_checksum() calls fgetc() to perform a checksum. There is non-trivial overhead per call. Consider re-write with a block of memory, say 256 or 4096, and fread().

The back and forth of reading Global checksum looks easy to do in one pass.

main() has too many details

I'd recommend making more helper functions.

Minor

(size_t)size relies on SIZE_MAX >= LONG_MAX. Common, but not certain. File sizes are not limited to SIZE_MAX.

\$\endgroup\$
6
  • \$\begingroup\$ If I tried to print a size_t constant such as SIZE_MAX using %zu, would I have to cast it to size_t? \$\endgroup\$
    – S.S. Anne
    Mar 29 '20 at 1:11
  • \$\begingroup\$ @S.S.Anne z was introduced in C99. Whenever printing some unsigned value lacking a matching print specifiers, a simple solution is to cast to widest type. In C89 that is unsigned long or printf("%lu\n", (unsigned long) nbytes); \$\endgroup\$ Mar 29 '20 at 4:05
  • \$\begingroup\$ I've given up on C89 since I have no reason whatsoever for using it. I was thinking about checking the size of the file against SIZE_MAX to see if it's safe to cast before doing so, and I wanted to print an error with the value of SIZE_MAX if it's too big. However, I think it might be possible that the constant might not have the same type as size_t, so I wanted to be sure that I'm using the correct type. \$\endgroup\$
    – S.S. Anne
    Mar 29 '20 at 14:43
  • \$\begingroup\$ In terms of a Game Boy cartridge, I doubt any practical concerns about file size. In general, a test of (unsigned long) nbytes > SIZE_MAX is prudent. The cast is to quiet a potential warning about signed/unsigned compare without changing values. Further, a subtle issue occurs when the file is larger than LONG_MAX. To handle that, code needs to find the file length in another way. \$\endgroup\$ Mar 29 '20 at 15:18
  • \$\begingroup\$ result + ~c This is fishy code, please note that c is int and the ~ almost certainly turns it into a negative number. Doing arithmetic on that is risky, in theory it would become something like 0 + INT_MIN when c is zero. \$\endgroup\$
    – Lundin
    Mar 31 '20 at 13:12
6
\$\begingroup\$

Error printing

You do the right thing in some cases:

if(fseek(fp, -2, SEEK_CUR) == -1)
    perror("fseek");

but not others:

if(fgetpos(fp, &pos) == -1)
    return -1;

Also, that particular check does not adhere to the specification, which says:

Upon successful completion, fgetpos() shall return 0; otherwise, it shall return a non-zero value and set errno to indicate the error.

C89/C99

We've been over this a little bit in the comments, but unless there is a specific target you have in mind that requires C89, it's best to go with something more modern. I generally use C18 but C99 is also a safe bet.

Among other things, this will buy you the ability to declare and initialize variables much closer to where you actually use them in the code, something that I find helps with legibility and maintainability.

Enum offsets

This isn't a critique, but a compliment: I had forgotten (or maybe never knew?) that enum values can be computed against each other, like

MISC = ENTRY + 0x34,

That's really cool. Keep doing that.

Double-cast

(unsigned char)~(unsigned char)c;

The rightmost cast is not necessary. Whereas inversion does change the type of a term to int (TIL), it is safe to do the inversion on the character directly, and then cast it after.

Gotos

Sometimes I find that there's actually a valid application of goto; I have a few toes outside of the never-goto camp. But I don't think that's the case here. Your use of goto can be easily avoided by factoring out a function that does early-return on failure.

\$\endgroup\$
12
  • \$\begingroup\$ Darn man-page says "Upon successful completion, fgetpos(), fseek(), fsetpos() return 0, and ftell() returns the current offset. Otherwise, -1 is returned and errno is set to indicate the error." \$\endgroup\$
    – S.S. Anne
    Mar 28 '20 at 16:27
  • 1
    \$\begingroup\$ Interesting - it could be that the man page is about a more specific implementation (GNU C?), in which case, believe the man page over the spec. \$\endgroup\$
    – Reinderien
    Mar 28 '20 at 16:28
  • 3
    \$\begingroup\$ != 0 still works so I'll go with that. I've found that the man-pages don't always match the spec. \$\endgroup\$
    – S.S. Anne
    Mar 28 '20 at 16:29
  • \$\begingroup\$ The double cast is necessary: tio.run/… \$\endgroup\$
    – S.S. Anne
    Mar 28 '20 at 16:40
  • 1
    \$\begingroup\$ A thought on C89 reviews: Not only has C been updated 20+ years ago, the what is proper for C89 gets fuzzier each year as the few compilers still adhering to that have added some of C99, etc. Also those who truly know C89 are fewer and have more cobwebs to sift to that - ahem romantic - era. \$\endgroup\$ Mar 29 '20 at 0:37
4
\$\begingroup\$

Here are some things that may help you improve your code.

Fix the bug

The program starts reading the header as though the file pointer were already pointing to location 0x100. That's an error because in all of the Gameboy files I've ever seen, the first 0x100 bytes are present and needed for calculating the checksum.

Use a struct where appropriate

This code could be made much simpler just by using a struct to represent the header. I'd use this:

struct CartridgeHeader {
    uint8_t filler[0x100];
    uint8_t entry[4];
    uint8_t logo[0x30];
    uint8_t title[0x10];
    uint8_t licensee[2];
    uint8_t SGB;
    uint8_t cart_type;
    uint8_t ROM_size;
    uint8_t RAM_size;
    uint8_t dest_code;
    uint8_t old_licensee_code;
    uint8_t mask_ROM_version;
    uint8_t header_checksum;
    uint8_t checksum_hi;
    uint8_t checksum_lo;
};

We can ignore most of these for the purposes of this program, but it only takes a minute to create the whole thing and parts of it might be useful for other purposes.

As noted in some of the comments, this assumes that the structure is not padded and that is not guaranteed by the standard. Many compilers include something like #pragma pack. If yours does, use it. Also, if your compiler supports C11, add this line to assure (at compile time) that the struct is what it needs to be:

static_assert(sizeof(struct CartridgeHeader) == 0x150, 
         "Code relies on struct having no padding");

Be efficient in file I/O

Instead of jumping around back and forth in the file, I'd suggest that a much cleaner approach would be to simply read the file once and then make a single write to update the file if needed.

Understand the header specification

The header specification says that the offset usually contains "NOP; JP 0150h" but not always. For that reason, it's not technically correct to check for those specific instructions there. The only thing the Gameboy checks for is the logo portion.

Avoid goto fail

While it may seem appealing, the goto fail as a technique is hazardous especially, as in this program, if you don't always use {} with if and for. It's difficult to make sure it is done correctly and easy to make a catastrophic error that makes international news as with Apple's infamous goto fail error. That's not something you want to be known for!

Avoid #define if you can

The problem with using a #define for data is that there is no type and therefore no type checking. Instead, you can better accomplish what you need with something like this:

static const uint8_t logo[] = {
/* Nintendo logo */ \
    0xce,0xed,0x66,0x66,0xcc,0x0d,0x00,0x0b,0x03,0x73,0x00,0x83,0x00,0x0c,0x00,0x0d, 
    0x00,0x08,0x11,0x1f,0x88,0x89,0x00,0x0e,0xdc,0xcc,0x6e,0xe6,0xdd,0xdd,0xd9,0x99,
    0xbb,0xbb,0x67,0x63,0x6e,0x0e,0xec,0xcc,0xdd,0xdc,0x99,0x9f,0xbb,0xb9,0x33,0x3e
};

Separate I/O from calculations where practical

If, as suggested above, we already have a struct, it would make sense to do the calculations on it in memory rather than as the values are being read. Here's one way to implement such a function:

static uint8_t cart_header_checksum(const struct CartridgeHeader *ch) {
    uint8_t sum = 0;
    for (uint8_t *ptr = (uint8_t *)&ch->title; ptr != &ch->header_checksum; ++ptr) {
        sum += ~*ptr;
    }
    return sum;
}

Think about more informative error return values

Most modern operating systems employ the use of an error value that can be returned from main. I'd suggest that instead of just pass/fail it might be useful if the program returned an error code suggesting what the problem was. It might look like this as an enum:

enum error_code { ERROR_NONE, ERROR_READ, ERROR_LOGO, ERROR_WRITE };

Think of the user

Rather than exiting the program with an error, I think it would be more useful to a user if the program simply told me that the checksums were correct already (if they are). If they're not, one might also want to know what values were originally and what the corrected values are. There's no need for the values to be required to be zero.

Putting it all together

Here's an alternative version that uses all of these ideas:

#include <assert.h>
#include <errno.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

struct CartridgeHeader {
    uint8_t filler[0x100];
    uint8_t entry[4];
    uint8_t logo[0x30];
    uint8_t title[0x10];
    uint8_t licensee[2];
    uint8_t SGB;
    uint8_t cart_type;
    uint8_t ROM_size;
    uint8_t RAM_size;
    uint8_t dest_code;
    uint8_t old_licensee_code;
    uint8_t mask_ROM_version;
    uint8_t header_checksum;
    uint8_t checksum_hi;
    uint8_t checksum_lo;
};

static_assert(sizeof(struct CartridgeHeader) == 0x150, "Code relies on struct having no padding");


static uint8_t cart_header_checksum(const struct CartridgeHeader *ch) {
    uint8_t sum = 0;
    for (uint8_t *ptr = (uint8_t *)&ch->title; ptr != &ch->header_checksum; ++ptr) {
        sum += ~*ptr;
    }
    return sum;
}

static bool cart_check_logo(const struct CartridgeHeader *ch) {
    static const uint8_t logo[] = {
    /* Nintendo logo */ \
        0xce,0xed,0x66,0x66,0xcc,0x0d,0x00,0x0b,0x03,0x73,0x00,0x83,0x00,0x0c,0x00,0x0d, 
        0x00,0x08,0x11,0x1f,0x88,0x89,0x00,0x0e,0xdc,0xcc,0x6e,0xe6,0xdd,0xdd,0xd9,0x99,
        0xbb,0xbb,0x67,0x63,0x6e,0x0e,0xec,0xcc,0xdd,0xdc,0x99,0x9f,0xbb,0xb9,0x33,0x3e
    };
    return memcmp(&ch->logo, logo, sizeof(logo)) == 0;
}

enum error_code { ERROR_NONE, ERROR_READ, ERROR_LOGO, ERROR_WRITE };

int main(const int argc, const char *const *const argv)
{
    if(argc != 2)
    {
        printf("Usage: %s gb-file\n", argv[0]);
        return ERROR_READ;
    }

    FILE *const fp = fopen(argv[1], "r+b");
    if(fp == NULL)
    {
        perror(argv[1]);
        return ERROR_READ;
    }

    struct CartridgeHeader header;
    if (fread(&header, 1, sizeof(header), fp) != sizeof(header)) {
        puts("Short file: header read failed");
        fclose(fp);
        return ERROR_READ;
    }
    if (!cart_check_logo(&header)) {
        puts("Logo verification failed; is this a valid file?");
        fclose(fp);
        return ERROR_LOGO;
    }

    // calculate header checksum
    uint8_t mysum = cart_header_checksum(&header);

    // calculate global checksum
    uint16_t global_sum = mysum;
    // first over part we alredy read
    for (uint8_t *ptr = (uint8_t *)&header; ptr != &header.header_checksum; ++ptr) {
        global_sum += *ptr;
    }
    // then continue with rest of file
    for (int ch = fgetc(fp); ch != EOF; ch = fgetc(fp)) {
        global_sum += ch;
    }

    if (mysum == header.header_checksum && global_sum == header.checksum_hi * 256 + header.checksum_lo) {
        puts("Cartridge already has valid checksums: nothing to do");
    } else {
        printf("calculated header checksum = %2.2x\n", mysum);
        printf("file header checksum = %2.2x\n", header.header_checksum);
        printf("calculated global sum = %4.4x\n", global_sum);
        printf("file global sum = %2.2x%2.2x\n", header.checksum_hi, header.checksum_lo);
        puts("Updating checksums");
        header.header_checksum = mysum;
        header.checksum_hi = global_sum >> 8;
        header.checksum_lo = global_sum && 0xff;
        if (fseek(fp, 0L, SEEK_SET) || fwrite(&header, 1, sizeof(header), fp) != sizeof(header)) {
            perror("Unable to write to file");
            return ERROR_WRITE;
        }
    }
    fclose(fp);
    return ERROR_NONE;
}

A few more notes

The code above reads most of the file using fgetc character at a time. While this may seem slow, modern operating systems typically use buffering and so this is not as slow as it might first seem. Another note is the code above does not attempt to distinguish between EOF and an actual file read error. This might happen, if, for example, the file is on removeable media and gets ejected during the process. This would lead to a failure of the fseek which is the next operation and so while the error message might be a bit misleading, it seemed to me not worth the bother to do anything differently. Such error checking could be added with a call to ferror if desired.

\$\endgroup\$
7
  • \$\begingroup\$ "The program starts reading the header as though the file pointer were already pointing to location 0x100. That's an error because in all of the Gameboy files I've ever seen, the first 0x100 bytes are present and needed for calculating the checksum." Yes, they are. I've fixed that by now. \$\endgroup\$
    – S.S. Anne
    Mar 30 '20 at 17:10
  • \$\begingroup\$ That struct would be hard to read to if structure elements are not stored sequentially in memory (which they're not guaranteed to be). \$\endgroup\$
    – S.S. Anne
    Mar 30 '20 at 17:18
  • \$\begingroup\$ It's true that it's not guaranteed by the C standard that items are stored sequentially in memory. While many compilers support something like #pragma pack it's not portable by definition. One could also write a custom reader or simply declare it as an 0x150 byte array of uint8_t (and use custom access methods) for guaranteed portability. \$\endgroup\$
    – Edward
    Mar 30 '20 at 17:24
  • \$\begingroup\$ In the time since I posted the question I changed the program so it reads the whole file in and only writes the three checksum bytes. \$\endgroup\$
    – S.S. Anne
    Mar 30 '20 at 17:27
  • 1
    \$\begingroup\$ Yes, that's another perfectly valid way to do it. Using that method, one could even calculate both the header checksum and the global checksum in the same pass. \$\endgroup\$
    – Edward
    Mar 30 '20 at 17:29
0
\$\begingroup\$

In addition to remarks by previous reviews:

  • This is wrong:

    int main(const int argc, const char *const *const argv)
    

    The form of main() is for the compiler to decide, not the programmer. This form is not at all compatible with standard C int main(int argc, char *argv[]). So unless your compiler docs specifically tell you that your custom form of main() is ok, you are invoking undefined behavior.

  • Regarding the #define HEADER_DATA, you have a subtle but severe bug, namely that each line introduces a null terminator \x00 since the data is string literals. This may screw up all CRC calculations and memcmp calls, if you don't take it in account. Wiser to roll with static const uint8_t as advised in another review.

    If you can compile as standard C, use lots of _Static_assert. For example your enum could end with a dummy entry END and that one should be the same as the size of the data, or otherwise your constants are corrupt somewhere.

  • for( result = 0; nbytes--; ) should be for(result = 0; nbytes>0; nbytes--) or you will get very strange results when passing nbytes == 0 to the function.

  • The result += (unsigned char)~(unsigned char)c; is code smell and the other reviews didn't get this quite right. This is what actually happens:

    • (unsigned char)c you explicitly convert from int to unsigned char.
    • ~ the compiler spots this operator and immediately and silently integer promotes back to int.
    • The result of ~ is of type int and very likely a negative number.
    • Casting to unsigned char again means that you parse out one byte from this negative number in an implementation-defined way. In practice, this will probably work just fine on most systems.
    • result += op is equivalent to result = result + op except result is only evaluated once (which doesn't matter here). Since both operands of + are small integer types, the result of + is int, but it can't be negative. You then lvalue convert this temporary int back into unsigned char upon assignment.

    Summary: way too much implicit crap going on here! C is dangerously subtle, particularly when it comes to the ~ << >> operators. There is no bug here, but this code is brittle. For rugged code with a minimum of implicit conversions, I would change this to:

    result += ~(unsigned int)c; or if you prefer result += ~(uint32_t)c;.

    This contains 2 silent promotions, result up to unsigned int and then the result of that back to unsigned char. All operands remain unsigned types, so it is harmless. More importantly, this should be fail-safe on traditional simple 8 bit checksums that rely on 8 bit unsigned wrap-around.

\$\endgroup\$
3
  • \$\begingroup\$ Each line does not introduce a null character. Only the last one does, since they are concatenated. This is handled where noted. \$\endgroup\$
    – S.S. Anne
    Mar 31 '20 at 15:46
  • \$\begingroup\$ for( result = 0; nbytes--; ) will handle nbytes == 0. It's a post-increment, so it breaks from the loop without doing anything. \$\endgroup\$
    – S.S. Anne
    Mar 31 '20 at 15:50
  • \$\begingroup\$ I would change it to result = result - c - 1;, which I've already done. \$\endgroup\$
    – S.S. Anne
    Mar 31 '20 at 15:57

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.