Tags: char, combine, created, function, path_combine, paths, programming, safe, target, viable, youint

path_combine

On Programmer » C & C++

3,075 words with 3 Comments; publish: Tue, 04 Dec 2007 14:34:00 GMT; (20063.48, « »)

Hey,

I created a function to combine two paths into one. Just wondering, does it look safe and viable to you?

int path_combine(char* target, char* source, char* combine)

{

if((strlen(source) + strlen(combine) + 10) > MAX_PATH_LENGTH) { return -1; }

do {

*target++ = *source++;

if(!*source) {

if(*--target == '/') {

if(*combine == '/')

*target = (char)0;

else {

target++;

}

}

else {

target++;

if(*combine != '/') {

*target = '/';

*target++;

}

}

}

} while(*source);

while(*combine)

*target++ = *combine++;

*target = (char)0; /* terminate the string */

return 0;

}

Cheers,

:afrog:

All Comments

Leave a comment...

  • 3 Comments
    • 1) Try Being Const Correct (http://cprogramming.com/tutorial/const_correctness.html)

      2) I don't think you need a cast here

      *target = (char)0;

      #1; Wed, 05 Dec 2007 14:45:00 GMT
    • It's correct (as far as I can tell), but it's neither safe nor efficient. I also find it very hard to understand, so I'm not entirely sure about the correctness.

      For safety, you really would need to pass in the buffer size of target and keep checking it.

      For efficiency, your biggest problem is the double check for source being at the end. But also the design of the function. I'd rather write a pathcat function that works like strcat, but with proper slash combination. Something like this:

      /* Appends one path to another. cur_s is the buffer size of the first argument.

      Returns NULL if the target buffer is not large enough. */

      char *pathcat(char *cur, size_t cur_s, const char *app)

      {

      char *oldend;

      assert(cur);

      assert(app);

      while(*cur) {

      ++cur; --cur_s;

      }

      assert(cur_s > 0); /* If this fires, then the buffer isn't even long enough to hold the original string. */

      oldend = cur;

      if(cur[-1] != '/') {

      *cur = '/';

      ++cur;

      --cur_s;

      }

      if(*app == '/') ++app;

      /* Now the copy operation. */

      while(cur_s > 1 && *app) {

      *cur++ = *app++;

      --cur_s;

      }

      if(*app) {

      /* The size limit terminated the loop. Fail. But restore the original string first. */

      *oldend = 0;

      return NULL;

      }

      /* Terminate the string. The test against 1 above guarantees we have the necessary space. */

      *cur++ = 0;

      return cur;

      }

      Note one thing: conditions usually are much slower than simple assignments. Thus, simply appending a slash to the old if there's none there and jumping the slash on the new if one is there is faster than detecting all four possible conditions and taking the "ideal" steps in all of them.

      Also, assertions are a really cool thing to catch programming errors in debug mode. Use them. Use them a lot. They're compiled away in release, so they're effectively free.

      #2; Wed, 05 Dec 2007 14:47:00 GMT
    • Thanks Bee :wave:
      #3; Wed, 05 Dec 2007 14:47:00 GMT