1
char *stringmult(int n)
{
    char *x = "hello ";
    for (int i=0; i<n; ++i)
    {
        char *y = new char[strlen(x) * 2];
        strcpy(y,x);
        strcat(y,x);
        delete[] x;
        x=y;
    }
    return x;
}

I'm trying to figure out what the flaws of this segment is. For one, it deletes x and then tries to copy it's values over to y. Another is that y is twice the size of x and that y never gets deleted. Is there anything that I'm missing? And also, I need to figure out how to get algorithm performance. If you've got a quick link where you learned how, I'd appreciate it.

9
  • Please post the complete code. Commented Jul 8, 2009 at 5:59
  • 6
    Sweet Zombie Jesus that is evil code. Commented Jul 8, 2009 at 5:59
  • 3
    @Earwicker: That makes it 'C with Classes'. I wouldn't call this C++ at all. Commented Jul 8, 2009 at 6:04
  • 3
    @rlbond: Putting everything under static void main(String[]) in Java won't make it C; use C++ like C don't make it less C++. Commented Jul 8, 2009 at 6:19
  • This evil code will be very useful for job interviews. It's just great. Commented Jul 8, 2009 at 7:41

7 Answers 7

5

y needs one more byte than strlen(x) * 2 to make space for the terminating nul character -- just for starters.

Anyway, as you're returning a newed memory area, it's up to the caller to delete it (eek).

What you're missing, it seems to me, is std::string...!-)

As for performance, copying N characters with strcpy is O(N); concatenating N1 characters to a char array with a previous strlen of N2 is O(N1+N2) (std::string is faster as it keeps the length of the string in an O(1)-accessible attribute!-). So just sum N+N**2 for N up to whatever your limit of interest is (you can ignore the N+ part if all you want is a big-O estimate since it's clearly going to drop away for larger and larger values of N!-).

Sign up to request clarification or add additional context in comments.

1 Comment

Even more "eek"; When called with n=0 caller shouldn't delete it. :-)
3

For starters delete[] x; operates for the first time round the loop on some static memory. Not good.

It looks like an attempt to return a buffer containing 2^n copies of the string "hello ". So the fastest way to do that would be to figure out the number of copies, then allocate a big enough buffer for the whole result, then fill it with the content and return it.

void repeat_string(const std::string &str, int count, std::vector<char> &result)
{
    result.resize(str.size() * count);
    for (int n = 0; n < count; n++)
        str.copy(&result[n * s.size()], s.size());
}

void foo(int power, std::vector<char> &result)
{
    repeat_string("hello ", 1 << (power + 1), result); 
}

Comments

2
  1. no need to call strlen() in a loop - only call it once;
  2. when new is called no space is requested for the null-character - will cause undefined behaviour;
  3. should use strcpy instead of strcat - you already know where to copy the second string and findig the end of string by strcat requires extra computation;
  4. delete[] is used on a statically allocated string literal - will cause undefined behaviour;
  5. memory is constantly reallocated although you know the result length well in advance - memory reallocation is quite expensive

You should instead compute the result length at once and allocate memory at once and pass the char* as an in-parameter:

char* stringMult(const char* what, int n)
{
     const size_t sourceLen = strlen( what );
     int i;
     size_t resultLen = sourceLen;
     // this computation can be done more cleverly and faster
     for( i = 0; i < n; i++ ) {
        resultLen *= 2;
     }
     const int numberOfCopies = resultLen / sourceLen;
     char* result = new char[resultLen + 1];
     char* whereToWrite = result;
     for( i = 0; i < numberOfCopies; i++ ) {
        strcpy( whereToWrite, what );
        whereToWrite += sourceLen;
     }
     return result;
}

Certain parts of my implementation can be optimized but still it is much better and (I hope) contains no undefined-behaviour class errors.

Comments

1

you have to add one while allocating space for Y for NULL terminating string Check the code at below location http://codepad.org/tkGhuUDn

1 Comment

hmmm.. char * target = malloc(source_len*m+1) * sizeof(char));
0
char * stringmult (int n)
{
    int i;
    size_t m;
    for (i = 0, m = 1; i < n; ++i; m *= 2);
    char * source = "hello ";
    int source_len = strlen(source);
    char * target = malloc(source_len*m+1) * sizeof(char));
    char * tmp = target;
    for (i = 0; i < m; ++i) {
        strcpy(tmp, source);
        tmp += source_len;
    }
    *tmp = '\0';
    return target;
}

Here a better version in plain C. Most of the drawbacks of your code have been eliminated, i.e. deleting a non-allocated pointer, too many uses of strlen and new. Nonetheless, my version may imply the same memory leak as your version, as the caller is responsible to free the string afterwards.

Edit: corrected my code, thanks to sharptooth.

4 Comments

That will replicate the string n times and the OP's code replicates it 2^n times.
And you call strlen and strcpy too often - this can be done much much faster.
Much better except that you should use size_t for the "number of elements" variables. Otherwise this code in unportable.
That's true, you have to call strcpy only n times, but the underlying copying takes the same time, since the strings will become bigger. It depends on the cost of calling strcpy if it is needed to further optimize the code.
0

char* string_mult(int n)

{

const char* x = "hello ";

char* y;

    int i;



for (i = 0; i < n; i++)

{

    if ( i == 0)

    {

        y = (char*) malloc(strlen(x)*sizeof(char));

        strcpy(y, x);

    }

    else

    {

        y = (char*)realloc(y, strlen(x)*(i+1));

        strcat(y, x);

    }

}

return y;

}

1 Comment

This hurts my eyes -- put the special case outside the loop and run from 1 to n instead :)
0

Nobody is going to point out that "y" is in fact being deleted?

Not even one reference to Schlmeiel the Painter?

But the first thing I'd do with this algorithm is:

int l = strlen(x);
int log2l = 0;
int log2n = 0;
int ncopy = n;
while (log2l++, l >>= 1);
while (log2n++, n >>= 1);
if (log2l+log2n >= 8*(sizeof(void*)-1)) {
    cout << "don't even bother trying, you'll run out of virtual memory first";
}

7 Comments

Emm... Where? x is being deleted, then y is copied into x.
@sharptooth: Exactly, so in the next iteration guess what was x... Bingo! the y from the previous iteration! Don't bother to say that then there would be a leak in the last, because that's the memory being returned.
Nope, on the next iteration y will be attached to a newly allocated memory block, then x will be deleted, y will be copied into x. This thing works fine. And there's no leak until the caller forgets to free memory.
Nope what? x and y are pointers, so doing y = new...; x = y; delete x; effectively deletes the memory that was allocated for y. I cannot see your point.
That would be the case if x=y was done before delete, but in the OP's code first delete is done, then x=y.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.