Sign in to follow this  
Followers 0
Richard Robertson

delete[] operator

18 posts in this topic

Maybe I'm not understanding, but when an array is allocated using say,

char *temp = new char[10 /* or any other int for that matter*/];

I would then delete using the vector delete operator

delete[] temp;

right?

I am attempting to write a sort of StringFormat by wrapping sprintf. I am accepting std::string parameters which sprintf does not use.

But regardless, my problem comes in that it freezes up at the line where I delete temp. Does anyone know why this would happen? According to my debugger, temp has a valid pointer and does indeed point to the value it should have at that moment. No matter what length of time I let it sit, it does not get past the delete[] operator.

I can post the entire function if necessary.

Share this post


Link to post
Share on other sites



#3 ·  Posted (edited)

#include <string>
#include <cstdarg>

/*  Return a formatted string consistant with sprintf
*       by use of sprintf, but with std::string type        
*   NoError
*
*   NOTE: Make sure the parameters match the output style
*       as no type checking can be performed
*   NOTE: Supports the types c,i,o,u,x,X,e,E,f,g,G,a,A,p,s,S
*       where s is std::string
*       and S is char * (C string)
*/
inline std::string StringFormat(std::string format, ...)
{
    va_list args;
    va_start(args, format);
    std::string ret = "";
    bool found = false;
    std::string types = "ciouxXeEfgGaApsS", temp = "";
    int temp2;
    char *temp3, type;
    for (unsigned int a = 0; a < format.length(); a++)
    {
        if (format[a] != '%')
            ret += format[a];
        else
        {
            found = false;
            temp = "%";
            while (!found)
            {
                a++;
                found = types.find(format[a]) != std::string::npos;
                temp += format[a];
            }
            type = format[a];
#pragma warning (push)
#pragma warning (disable: 4996)
            if (temp.find("*") != std::string::npos)
            {
                temp2 = va_arg(args, int);
#define _quick(t, t_) {t##t_ item##t = va_arg(args, t##t_);\
temp3 = new char[_scprintf(temp.c_str(), temp2, item##t)];\
sprintf(temp3, temp.c_str(), temp2, item##t);}\
break
                switch (type)
                {
                case 'c':
                    _quick(char, );
                case 'i':
                case 'o':
                case 'u':
                case 'x':
                case 'X':
                    _quick(int, );
                case 'e':
                case 'E':
                case 'f':
                case 'g':
                case 'G':
                case 'a':
                case 'A':
                    _quick(double, );
                case 'p':
                    _quick(void, *);
                case 's':
                    {std::string itemstring = va_arg(args, std::string);
                    temp3 = new char[_scprintf(temp.c_str(), temp2, itemstring.c_str())];
                    sprintf(temp3, temp.c_str(), temp2, itemstring.c_str());}
                    break;
                case 'S':
                    {char *itemchara = va_arg(args, char *);
                    temp3 = new char[_scprintf(temp.replace(temp.length() - 1, 1, "s").c_str(), temp2, itemchara)];
                    sprintf(temp3, temp.replace(temp.length() - 1, 1, "s").c_str(), temp2, itemchara);}
                    break;
                }
#undef _quick
            }
            else
            {
#define _quick(t, t_) {t##t_ item##t = va_arg(args, t##t_);\
temp3 = new char[_scprintf(temp.c_str(), item##t)];\
sprintf(temp3, temp.c_str(), item##t);}\
break
                switch (type)
                {
                case 'c':
                    _quick(char, );
                case 'i':
                case 'o':
                case 'u':
                case 'x':
                case 'X':
                    _quick(int, );
                case 'e':
                case 'E':
                case 'f':
                case 'g':
                case 'G':
                case 'a':
                case 'A':
                    _quick(double, );
                case 'p':
                    _quick(void, *);
                case 's':
                    {std::string itemstring = va_arg(args, std::string);
                    temp3 = new char[_scprintf(temp.c_str(), itemstring.c_str())];
                    sprintf(temp3, temp.c_str(), itemstring.c_str());}
                    break;
                case 'S':
                    {char *itemchara = va_arg(args, char *);
                    temp3 = new char[_scprintf(temp.replace(temp.length() - 1, 1, "s").c_str(), itemchara)];
                    sprintf(temp3, temp.replace(temp.length() - 1, 1, "s").c_str(), itemchara);}
                    break;
                }
#undef _quick
#pragma warning (pop)
            }
            ret += temp3;
            delete[] temp3;
            temp3 = 0;
        }
    }
    return ret;
}

Yes, I know the code is messy as hell.

I am allocating a buffer (temp3) by calling _scprintf to make sure I have exactly the right size. temp3 gets allocated correctly and contains the correct value after a call to sprintf, but the entire thing locks up at delete[]. ret even contains temp3 added on to the end.

I think I have made no syntax mistakes, and according the compiler, I haven't either, but I'm not sure what's wrong here. When It freezes, I can break into the debugger and the stack trace is sitting at delete[].

Edited by Richard Robertson

Share this post


Link to post
Share on other sites

#4 ·  Posted (edited)

Why are you doing that by hand at all? vsprintf() and _vscprintf() are your friends. It's trivial to get the length, allocate a buffer, use vsprintf() to populate it and return the buffer as a std::string. Should be like 10 lines of code or something.

Edit: Ahh, I see, you're trying to change %s to be a std::string. May I ask, why? Seems rather pointless to me. Yes, that code is messy and no offense but poorly written. The pragmas, #define, et cetera make it really really hard to read/debug.

Edited by Valik

Share this post


Link to post
Share on other sites

#5 ·  Posted (edited)

Because my teacher wants us to use std::string over char pointers. That's kind what it boils down to.

I've never heard of vsprintf though. I'll look at that and hopefully it's much better than what I have now.

The big thing is, is that I can't figure out why it would freeze up at delete[]. I mean, it shouldn't, should it?

Edit: After searching for vsprintf, it will make things a lot easier for me. I still have to deal with my std::string's but now I don't have to deal with the stack by hand. Thanks Valik.

Oh and, I know it's poorly written. I take no offense from that.

Edited by Richard Robertson

Share this post


Link to post
Share on other sites

You still have to deal with the stack by hand. Remember, the stack contains a std::string but vsprintf() doesn't know how to handle that. vsprintf() can't be coaxed into understanding what you want to do with a std::string. There is no way to get vsprintf() to skip it, either. The only way to actually use vsprintf() is to pass it only so many args, stop, skip past the std::string, use it again until the next std::string, et cetera. Messy, possibly.

Share this post


Link to post
Share on other sites

As it still falls back to what I'm doing here.

The problem I can't figure out is why delete[] isn't working. I'm gonna try ripping up and rewriting my function and seeing what I can do.

Share this post


Link to post
Share on other sites

If a memory operation like delete fails, or you get a crash on exit it's usually because you've overrun a buffer somewhere. Have you stepped through the code line by line rather than breaking in when it's already crashed?

Share this post


Link to post
Share on other sites

It doesn't crash, it just stops. When I hit break, it says that it's running in a non-user interactive mode. I can try walking the code though to see what happens.

Share this post


Link to post
Share on other sites

Personally I'm not a fan of complicated lines like:

temp3 = new char[_scprintf(temp.replace(temp.length() - 1, 1, "s").c_str(), itemchara)];

Although it looks like you might be optimizing (instead of using a temp variable to hold the result of _scprintf) if you look at the resulting assembler you might be surprised. It makes it twice as hard to read/debug as well.

Share this post


Link to post
Share on other sites

It doesn't crash, it just stops. When I hit break, it says that it's running in a non-user interactive mode. I can try walking the code though to see what happens.

delete[] doesn't just stop... What is the CPU like at the time? Maxed? Are you not just in a neverending loop?

Share this post


Link to post
Share on other sites

#12 ·  Posted (edited)

I figured it out. When you mentioned writing past the end of the buffer, and I was walking through, I found an embedded error message in the CRT files during my trace of the stack when I broke into the code. It said that something was written past the end of the heap.

I looked at the documentation for _scprintf and it doesn't account for the terminating NULL character.

For the sake of answering the question, I set a breakpoint after the delete[] line and it never got to it. It stopped, and had an error, but for some reason failed to display it.

Thanks for the input you two.

Edit: I found more errors in my code, but I am fixing them with much ease compared to this. Oh what convoluted code I have conceived.

Edited by Richard Robertson

Share this post


Link to post
Share on other sites

Personally I'm not a fan of complicated lines like:

temp3 = new char[_scprintf(temp.replace(temp.length() - 1, 1, "s").c_str(), itemchara)];

Although it looks like you might be optimizing (instead of using a temp variable to hold the result of _scprintf) if you look at the resulting assembler you might be surprised. It makes it twice as hard to read/debug as well.

And how did Jon learn that about temporaries? :)

Jon's right, however. The compiler is really good at optimizing away temporaries. Rule #1: Write clean code. Rule #2: Write optimized code. In that order, most of the time.

Also, you don't need to use #define at all. In fact, since this is for class, you may be hurting your grade by using it. Use inline functions instead. There's no need to use a #define to create those small little snippets. In fact, what you wrote is a template, so use a C++ template.

Share this post


Link to post
Share on other sites

How is it a template? I'm accepting an arbitrary amount of arguments. I don't know the type of each one ever. I just make assumptions based on the format string.

And I used #define in there because I got tired of typing. I will expand each of those later, but I just wanted to get a working solution.

Share this post


Link to post
Share on other sites

I'm talking about the code you have #defined. It is nothing more than a template.

Share this post


Link to post
Share on other sites

#16 ·  Posted (edited)

Oh, those. The #define was poor coding due to laziness.

For anyone who cares, I have rewritten the function and took the idea of a mini inline template function too.

#include <string>
#include <cstdarg>

namespace Internal
{
#pragma warning (push)
#pragma warning (disable: 4996) // non-secure sprintf

    // Used internally by StringFormat function, DO NOT CALL DIRECTLY
    template <class t> inline char *SmallFormat(const char *format, t item)
    {
        char *temp = new char[_scprintf(format, item) + 1];
        sprintf(temp, format, item);
        return temp;
    }

    // Used internally by StringFormat function, DO NOT CALL DIRECTLY
    template <class t> inline char *SmallFormat(const char *format, int width, t item)
    {
        char *temp = new char[_scprintf(format, width, item) + 1];
        sprintf(temp, format, width, item);
        return temp;
    }

#pragma warning (pop)
}

/*  Return a formatted string consistant with sprintf
*       by use of sprintf, but with std::string type        
*   NoError
*
*   NOTE: Make sure the parameters match the output style
*       as no type checking can be performed
*   NOTE: Supports the types c,i,o,u,x,X,e,E,f,g,G,a,A,p,s,S
*       where s is std::string
*       and S is char * (C string)
*/
inline std::string StringFormat(std::string format, ...)
{
    va_list args;
    va_start(args, format);
    std::string ret = "";
    bool found = false;
    std::string types = "ciouxXeEfgGaApsS", subformat = "";
    int width;
    char *subtemp, type;
    for (unsigned int a = 0; a < format.length(); a++)
    {
        if (format[a] != '%')
            ret += format[a];
        else
        {
            found = false;
            subformat = "%";
            while (!found)
            {
                a++;
                found = types.find(format[a]) != std::string::npos;
                subformat += format[a];
            }
            type = format[a];
            if (subformat.find("*") != std::string::npos)
            {
                width = va_arg(args, int);
                switch (type)
                {
                case 'c':
                    subtemp = Internal::SmallFormat(subformat.c_str(), width, va_arg(args, char));
                    break;
                case 'i':
                case 'o':
                case 'u':
                case 'x':
                case 'X':
                    subtemp = Internal::SmallFormat(subformat.c_str(), width, va_arg(args, int));
                    break;
                case 'e':
                case 'E':
                case 'f':
                case 'g':
                case 'G':
                case 'a':
                case 'A':
                    subtemp = Internal::SmallFormat(subformat.c_str(), width, va_arg(args, double));
                    break;
                case 'p':
                    subtemp = Internal::SmallFormat(subformat.c_str(), width, va_arg(args, void *));
                    break;
                case 's':
                    subtemp = Internal::SmallFormat(subformat.c_str(), width, va_arg(args, std::string).c_str());
                    break;
                case 'S':
                    subtemp = Internal::SmallFormat(subformat.replace(subformat.length() - 1, 1, "s").c_str(), width, va_arg(args, char *));
                    break;
                }
            }
            else
            {
                switch (type)
                {
                case 'c':
                    subtemp = Internal::SmallFormat(subformat.c_str(), va_arg(args, char));
                    break;
                case 'i':
                case 'o':
                case 'u':
                case 'x':
                case 'X':
                    subtemp = Internal::SmallFormat(subformat.c_str(), va_arg(args, int));
                    break;
                case 'e':
                case 'E':
                case 'f':
                case 'g':
                case 'G':
                case 'a':
                case 'A':
                    subtemp = Internal::SmallFormat(subformat.c_str(), va_arg(args, double));
                    break;
                case 'p':
                    subtemp = Internal::SmallFormat(subformat.c_str(), va_arg(args, void *));
                    break;
                case 's':
                    subtemp = Internal::SmallFormat(subformat.c_str(), va_arg(args, std::string).c_str());
                    break;
                case 'S':
                    subtemp = Internal::SmallFormat(subformat.replace(subformat.length() - 1, 1, "s").c_str(), va_arg(args, char *));
                    break;
                }
            }
            ret += subtemp;
            delete[] subtemp;
            subtemp = 0;
        }
    }
    return ret;
}

What do you know, I even renamed some variables and made it more readable.

Edited by Richard Robertson

Share this post


Link to post
Share on other sites

#18 ·  Posted (edited)

Yes, I fully agree with you there. I'm not a perfect coder, and neither is everyone else. I've been known to write some weird looking stuff though.

Oh and, it works now too, after adding an extra 1 char to the buffer to account for the null terminator.

Edited by Richard Robertson

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!


Register a new account

Sign in

Already have an account? Sign in here.


Sign In Now
Sign in to follow this  
Followers 0