Richard Robertson Posted February 28, 2008 Share Posted February 28, 2008 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. Link to comment Share on other sites More sharing options...
Administrators Jon Posted February 28, 2008 Administrators Share Posted February 28, 2008 The code above is fine - must be something else in the function. Deployment Blog: https://www.autoitconsulting.com/site/blog/ SCCM SDK Programming: https://www.autoitconsulting.com/site/sccm-sdk/ Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 28, 2008 Author Share Posted February 28, 2008 (edited) expandcollapse popup#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 February 29, 2008 by Richard Robertson Link to comment Share on other sites More sharing options...
Valik Posted February 28, 2008 Share Posted February 28, 2008 (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 February 28, 2008 by Valik Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 29, 2008 Author Share Posted February 29, 2008 (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 February 29, 2008 by Richard Robertson Link to comment Share on other sites More sharing options...
Valik Posted February 29, 2008 Share Posted February 29, 2008 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. Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 29, 2008 Author Share Posted February 29, 2008 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. Link to comment Share on other sites More sharing options...
Administrators Jon Posted February 29, 2008 Administrators Share Posted February 29, 2008 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? Deployment Blog: https://www.autoitconsulting.com/site/blog/ SCCM SDK Programming: https://www.autoitconsulting.com/site/sccm-sdk/ Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 29, 2008 Author Share Posted February 29, 2008 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. Link to comment Share on other sites More sharing options...
Administrators Jon Posted February 29, 2008 Administrators Share Posted February 29, 2008 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. Deployment Blog: https://www.autoitconsulting.com/site/blog/ SCCM SDK Programming: https://www.autoitconsulting.com/site/sccm-sdk/ Link to comment Share on other sites More sharing options...
Administrators Jon Posted February 29, 2008 Administrators Share Posted February 29, 2008 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? Deployment Blog: https://www.autoitconsulting.com/site/blog/ SCCM SDK Programming: https://www.autoitconsulting.com/site/sccm-sdk/ Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 29, 2008 Author Share Posted February 29, 2008 (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 February 29, 2008 by Richard Robertson Link to comment Share on other sites More sharing options...
Valik Posted February 29, 2008 Share Posted February 29, 2008 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. Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 29, 2008 Author Share Posted February 29, 2008 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. Link to comment Share on other sites More sharing options...
Valik Posted February 29, 2008 Share Posted February 29, 2008 I'm talking about the code you have #defined. It is nothing more than a template. Link to comment Share on other sites More sharing options...
Richard Robertson Posted February 29, 2008 Author Share Posted February 29, 2008 (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. expandcollapse popup#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 February 29, 2008 by Richard Robertson Link to comment Share on other sites More sharing options...
Administrators Jon Posted February 29, 2008 Administrators Share Posted February 29, 2008 It's 100 times more readble, if nothing else. Deployment Blog: https://www.autoitconsulting.com/site/blog/ SCCM SDK Programming: https://www.autoitconsulting.com/site/sccm-sdk/ Link to comment Share on other sites More sharing options...
Richard Robertson Posted March 1, 2008 Author Share Posted March 1, 2008 (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 March 1, 2008 by Richard Robertson Link to comment Share on other sites More sharing options...
Recommended Posts
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 accountSign in
Already have an account? Sign in here.
Sign In Now