1
\$\begingroup\$

I am thinking of using this as part of a header file to give me a useful string-manipulation tool. Elements is size of dest to prevent overflow:

int copystring(char *dest,char *source,int elements)
{
    int run,omitted=0;
    for(run=0;run<elements-1;run++)//last reserved for'\0'
    {
       dest[run]=source [run];
       if (dest[run]=='\0')
       { 
           break;
           //doesn't increment, so a below will be '\0'
       }
    }
    //now see if characters missed out, starting from first non-copied character 
    char a=source[run];
    while (a!='\0')
    {
       run++;//move on to next char in source
       omitted++;// missed out one more char
       a=source[run];
    }
    dest[elements-1]='\0';//could add an if condition if omitted is 0 
    return omitted;///tells parent function how many letters weren't copied
}

It may be worth my while adding a macro defining maximum amount of omitted chars to check for, and if exceeded to return -1.

Advantages: terminates in null charcter like strcpy while avoiding overflow like strncpy; returns the amount of characters, if any, that were omitted, so saves writing bit of code for handling this if needed.

\$\endgroup\$
1
\$\begingroup\$

Just a few comments.

  1. The standard type for specifying something like the maximum length of a string is size_t. You should probably use that instead of int.

  2. traversing the remainder of the string on the off chance that somebody might want to know how much was skipped seems like poor design. Unless you know you're going to use that (frequently), I'd advise against it. If somebody wants that information, strlen(s) - size is pretty trivial to type, so it's not like you're saving them something terribly arduous.

  3. I think most people accustomed to C would probably use pointer notation throughout.

    char *copystring(char *restrict dest, char const *restrict source, size_t elements) {
        char *d;
        for (d = dest; d + 1 < dest + elements; d++, source++)
            *d = *source;
        *d = '\0';
        return d;
    }
    
  4. This returns a pointer to the end of the new string, which frequently is quite useful. For example, if you're building up a string with data filled in at appropriate points, it's often useful to limit the size of each field you insert to a specific maximum, then get a pointer to where you're going to add the next item (though, in fairness, something like snprintf is often more convenient still).

\$\endgroup\$
  • \$\begingroup\$ hmmm, lots to think about. I was considering two versions, one with just 1 and 0 returns to let me know if all data has been used,-for instance to inform on program output- as well as this, but never thought about returning a pointer for the end, so that's something to consider. \$\endgroup\$ – user101969 Apr 28 '16 at 13:05
  • \$\begingroup\$ 1) This sample code has 2 compiler errors. suggest review. 2) Would prefer code that gracefully handles the corner case copystring(dest, source, 0) rather than going UB. \$\endgroup\$ – chux - Reinstate Monica Apr 29 '16 at 17:34
  • \$\begingroup\$ @chux: I've fixed the typos. Not sure there was any UB before, but I've modified the code to at least make it clear that the source and destination should be distinct. \$\endgroup\$ – Jerry Coffin Apr 29 '16 at 20:25
  • \$\begingroup\$ After switching from signed to unsigned for the buffer size, if elements == 0, the code has an integer overflow problem. \$\endgroup\$ – 5gon12eder Apr 29 '16 at 22:25
  • \$\begingroup\$ Hmmm, this doesn't look like a string copy function as the loop does not break when the null character of source is encountered. \$\endgroup\$ – chux - Reinstate Monica Jan 15 at 22:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy