Pointer version of strcat

Question

Write a pointer version of the function strcat that we showed in Chapter 2: strcat(s, t) copies the string t to the end of s.

This is the version of strcat from Chapter 2:

void strcat(char s[], char t[]) {
    int i, j;

    i = j = 0;
    while(s[i] != '\0') /* find the end of s */
        i++;

    while((s[i++] = t[j++]) != '\0') /*  copy t */
        ;
}

Here is my solution:

void custom_strcat(char *s, char *t) {
    while(*s) /* finding the end of the string */
        s++;

    while((*s++ = *t++)) /* copy t */
        ;
}

There are 2 while loops in my function, first loop will run until *s is equal to '\0'. At each iteration the value of s is incremented - thus, it will point to the next element;

The second loop will copy the elements of the string t at the and of s. The incrementation is done in the test part - even though after the loop stops s and t will point to other objects.This technique couldn't be applied in the first loop because when the loop stops s will point to an irrelevant value.

The exercise can be found at page 121 in K&R second edition.

Answer 1

It looks idiomatic and bug-free.

s++;

Perhaps ++s as a habit unless postfix s++ is required (as it is during the copy).

while((*s++ = *t++))

Could have just a single pair of parentheses.

void custom_strcat(char *s, char *t)

The signature of strcat is usually char* strcat(char* destination, const char* source). The const means that the source buffer is not modified. The return code is supposed to be a pointer to the original (unmodified) source value, so to implement this the implementation would need to increment a local variable copy of destination.

This technique couldn't be applied in the first loop because when the loop stops s will point to an irrelevant value.

You could write that as, if (*s) while (*(++s));.

Answer 2

I don't know much, but I would change a few things.

• Functions usually have the first { in a new line

• Explicit check against '\0'

• Make the second parameter const since the function won't change the characters it points to

• Use names more meaningful

This is what it would like:

void custom_strcat(char *dest, const char *source) 
{
    while(*dest != '\0') /* finding the end of the string */
         dest++;

     while((*dest++ = *source++) != '\0') /* copy source */
        ;
}

Answer 3

Your code looks fine for what it's intended to be. My biggest gripe against its implementation is the names s and t, as I keep reading s as source. However in the spirit of full code review, I wanted to mention I would disallow use of custom_strcat. Why? Well, it's never too early to learn a little bit about security; specifically buffer overflows.

The original strcat function is very easy to misuse because of one critical assumption that it makes: it assumes the caller has ensured the destination buffer is large enough to accept appending the source buffer's contents. When this assumption is false, many bad things can happen. If something external can control the source string, it may be able to exploit any assumptions that are invalidated by unexpected input. While a caller can check for strcat's assumption easily enough, other functions' assumptions (such as sprintf's) are not.

Typically this is countered today by passing a buffer-size parameter for any output buffer, and limiting the additions to the buffer such that they fit (see strlcat). In production code I would never want to see code that didn't include a way of ensuring buffer safety, and that's why I would disallow use of strcat or custom_strcat as written.

Answer 4

Overall, the code is acceptable and a common way to implement such algorithms. Below I have posted some things to consider:

Dangerous programming practice

• Pointer parameters that you are only going to use for reading, should be made read-only by using the keyword const. Google "const correctness". (Source: MISRA-C:2004 16.7)

• Always use {} braces for each statement, even if it only contains one row. If you don't, you are risking to get a classic code maintenance bug later on. Example:

  // original code;
  while(something)
    do_stuff();
  
  // the code is modified later on:
  while(something)
    do_stuff();
    do_more_stuff();
  

  As you can see, this introduced a severe bug, the programmer was tricked by the indention. This bug is incredibly common. (Source: MISRA-C:2004 14.8)

• Never use assignment inside conditions. In this specific case, it might be acceptable, but in other cases you want the compiler to give a warning if it encounters a = instead a condition, because that will prevent against another classic bug, namely accidental = where == was intended. Therefore it is good practice to avoid assignments inside conditions entirely. (Source: MISRA-C:2004 13.1)

• As others have pointed out, always do the test against null termination explicitly, by comparing against \0. while(*s) means: "either this code is perfectly fine as does what I intended", or it could mean that "I have written a bug here, because checking against NULL was my true intention". The reader can't tell. Show that you have actually done some thinking when you wrote the code, by explicitly add a check == '\0'. (Source: MISRA-C:2004 13.2)

• The ++ operators are dangerous to use combined with other operations, because they easily introduce various forms of poorly-defined behavior, such as the classic a[i] = i++ bug (undefined behavior) or unspecified behavior reliance issues like func(i) + func(i++) (the code relies on order of evaluation), or subtle operator precedence bugs like *ptr++ where (*ptr)++ was intended. The ++ and -- are simply plain dangerous to use anywhere except than on a line of its own. (Source: MISRA-C:2004 12.1, 12.2, 12.13)

Coding style

• Use intuitive, descriptive variable names. Don't use one-letter names. Commonly used names for copying functions are "dest" (destination) and "source".

  (Although the C standard itself uses poor variable naming, typically naming them "s1" and "s2". But don't use as bad coding style as they do in the C standard :) )

Improved code with all of the above in mind:

void custom_strcat (char* dest, const char* source) 
{
  while(*dest != '\0') /* finding the end of the string */
  {
    dest++;
  }

  *dest = *source;
  while(*dest != '\0')
  {
    dest++;
    source++;
    *dest = *source;
  }
}

---

(To avoid subjective debating and to demonstrate that these aren't just the personal, subjective opinions of a random internet person, I have cited a widely-acknowledged C programming authority as source for each statement made.)