Writing a string compare function

Are there any places where the efficiency can be improved? What other considerations need to be taken into account when comparing two strings, or does this cover all cases:

Efficient functions = leave error handling to the caller as much as possible. Well-written functions should focus on their designated purpose as much as possible. It is not the purpose of a string comparison function to sanitize pointers in the calling application, so why should it?

Ignoring the above program design aspect, it is still naive to write functions checking if pointer parameters are NULL or if they point at the same address etc. The caller might have perfect control over their data and know that it is valid - in which case your error checks only added lots of unnecessary bloat in the form of overhead code with branches.

What you should do instead is do add proper source code documentation of the function, explaining that it expects two pointers to valid data and that it does no error handling internally. Then a caller who suspect that their data isn't properly sanitized can add those NULL checks etc in the calling code. Which was the callers job all along, not yours.

Another important thing when it comes to efficiency, documentation and general good practice is to use const correctness. That is:

bool s_compare (const char* str1, const char* str2);

This tells the caller that the function won't modify the data, which is self-documenting code. It also tells the compiler that the data won't be modified, allowing various optimizations. For example it can assume that no external linkage variables will be modified by the function.

It also prevents the person writing the function from writing accidental bugs, though that argument is quite overrated in my opinion.

Furthermore, old school C comparison functions are written with the syntax

int func (const void* obj1, const void* obj2);

This form has the benefit that you can pass it to bsearch and qsort, so it might be a good idea to switch to this form instead, in case you need to search or sort the data. Here the function should return -1, 0 or 1 rather than just true/false, see documentation for bsearch etc.

Before you address efficiency or performance, you must ensure correctness. Your s_compare() function does not check if its arguments are null pointers. Whether such checks should be performed is a matter of specification: the standard strcmp() function requires valid pointers, but free() accepts null pointers and are specified as such.

From the comment, it seems the function should accept null pointers and return true if both arguments are null pointers. Yet it has undefined behavior if only one of its arguments is NULL.

There is one small issue int the posted code: variable i has type int, which is different from size_t and most likely has a smaller range. If the string length is MAX_INT or larger, the behavior is undefined. i should have type size_t.

Here are areas of improvement:

• As the function does not modify the strings, the arguments should have type const char * to allow passing constant strings without a compiler warning.

• the test for the empty string is redundant and saves a single comparison if str1 is empty while adding one in all other cases. It should be removed.

• testing for identical string pointers is also redundant but does improve performance for this particular case, reducing the complexity from linear to constant time. If s_compare is called with identical pointers very often, this might be worthwhile, but in the general case, it is not necessary, except for the special case of null pointers if specified.

• scanning the first string twice, once to compute its length and a second time to compare the characters with the second string simplifies the code but is counter productive: the first string is always read completely even when the difference would be found in the first few bytes. It seems much better to compare the contents and test for end of string progressively. Note that comparing to '\0' only needs to happen if the characters at identical.

Here is a modified version:

bool s_compare(const char *str1, const char *str2) {
    // s_compare accepts null pointers and returns true if
    // both arguments are null
    if (!str1 || !str2)
        return str1 == str2;

    // iterate for each byte until a difference is found
    // return true if the identical byte is the null terminator
    for (size_t i = 0; str1[i] == str2[i]; i++) {
        if (str1[i] == '\0') 
            return true;
    }
    return false;
}