Issues with strcat()?

Started by
7 comments, last by Emmanuel Deloget 17 years, 10 months ago
Hello, thar. I'm currently working on a little OpenGL diddy. As of now trying to make a working "edit box". So you click on it, it's highlighted and you can add text by using your keyboard. I'm having a problem adding to the string that it is supposed to render, though. I made my editboxes a class, with of course what they have typed into them as a private variable (char*). Here's what I'm attempting to do:
void cGUI::addLetter(unsigned char letter)
{
   if(hwndFocus != 255 && editFocus != 255 && strlen(getWindow(hwndFocus)->vEditBox[editFocus]->data) < 11)
   {
      char str[80];
      strcpy(str, "Hello. ");
      strcat(str, "Hi.");
      getWindow(hwndFocus)->vEditBox[editFocus]->data = str;
   }
}
It works... to some extent. When I press a button, "Hello. Hi" appears in the edit box, however only for a split second. Then it disappears. I figured, maybe something is happening to the private variable? So I try doing
getWindow(hwndFocus)->vEditBox[editFocus]->data = "Hello. Hi."
This works perfectly. It keeps rendering and stays in the edit box. So I figured the problem must be with my strcat. I'm using strcat, since it's supposed to add "letter" to the end of the private variable. Thus we have typing. Any help is appreciated! --Aternus
Advertisement
You are copying the string to a local variable. At the end of the function, the local variable is gone, therefore so is your string.
Steve 'Sly' Williams  Monkey Wrangler  Krome Studios
turbo game development with Borland compilers
Sly got it in one. I believe you should use strcpy instead of that assignment to data.
Good, fast, cheap. Pick any two.
strcpy doesn't really seem to work with pointers, though. I changed the code to
{   if(hwndFocus != 255 && editFocus != 255 && strlen(getWindow(hwndFocus)->vEditBox[editFocus]->data) < 11)   {      //char str[] = "";      //strcpy(str, "Hello. ");      //strcat(str, "Hi.");      char* w = "";      char* h = "HI";      strcpy(w, h);      //strcpy(getWindow(hwndFocus)->vEditBox[editFocus]->data, str);   }}
And just that will crash the program. Let alone taking out the comments. o.o

--Aternus
No no, they mean instead of:

getWindow(hwndFocus)->vEditBox[editFocus]->data = str

do

strcpy(getWindow(hwndFocus)->vEditBox[editFocus]->data,str);

not what you have done. That won't work at all since you are creating a pointer called w that points to a constant single-char null-terminated string, then you are trying to copy "HI" over the top which won't work for a number of reasons.

Assuming your vEditBox has a char[] data member with enough space, the above will work but really, you would be better of using std::string for everything right up to where you pass it to OpenGL, then use the c_str() member to get a const char *pointer. If you were doing this, the orignal code would have worked. Plus, instead of usint strcpy and strcat, you would have just

std::string str="Hello. ";
str+="Hi.";
getWindow(hwndFocus)->vEditBox[editFocus]->data = str;

assuming vEditBox->data was also a std::string.
Since you are using C++, the best answer is not to use strcpy or strcat or anything which is related to C style ASCIIZ strings but to use std::string :)

data should be declared as a std::string - then your code would look like:
getWindow(hwndFocus)->vEditBox[editFocus]->data = std::string("Hello. ") + std::string("Hi.")
It is far simpler and far more robust.

On a side note, your code is rather dangerous: what if getWindow() returns NULL? What if editFocus is greater than the size of the edit box vector? You get it: your code will painfully die, and the world will collapse in a big crunch. To be more precise, code that looks like a->b->c should be avoided.

HTH,
See sig. :)

(Enigma and I assume that the type of this 'data' member is under your control)

The problem is that a pointer is *only a pointer*. The pointer itself knows absolutely nothing about what it's pointing at, or even if it's "allowed" to point where it currently does. The C-style char* "strings" only work by convention: they make the very dangerous assumptions that, at the location in memory where the pointer points, there exists some sequence of bytes of data, each of which represents a text character, which eventually end with a zero-valued byte that denotes the end of the string; and that all of the bytes in this sequence are bytes that "belong" to the program (see OS memory protection) and are a separate variable (i.e. not used for some other program variable). Sadly, these very dangerous ideas are built right into the language, via string literals. When you put some text in double-quotes, the compiler generates such a sequence of bytes, which is embedded into the actual executable file, and at run-time, the pointer will point to some location in memory that's in the middle of the in-memory copy of your executable file.

If you strcpy into a buffer that's not big enough - boom.
If you overrun a buffer with more text than can fit inside it - boom. Or if you fill it exactly, such that there's no room at the end for the zero byte ("null terminator") - still boom.
If you try to do basically anything with a pointer that's either null, or not initialized - boom. (Also note that there is no standard way in code to look at a pointer value and determine whether it really points at the location that it claims to, or is uninitialized and simply happens to have whatever weird value.)
If you try to do any kind of writing to the string that a pointer points at, if it's pointing at a string literal - boom. (If you write past the original length of the string, you're writing on who-knows-what in the middle of your executable; but even if you respect the original string length, you might not be allowed to write to that memory because of OS memory protection, and even if you're allowed to write it, the compiler is allowed but not required to have two string literals of the same text represented by the same string, so you may or may not affect "other" literals that you didn't intend to.) The compiler is *supposed* to be able to warn you about this one, via the "const correctness" mechanism, but it can't because of C backwards compatibility problems.

There are plenty more "booms" that come up when you try to work around the problems (by dynamic memory allocation). And with *any* of these "booms", technically *anything* can happen, because the compiler is allowed to assume you know what you're doing. As far as it's concerned, these issues are Somebody Else's (i.e. your) Problem.

It's not worth it. Just use std::string.
Alright, thanks a bunch. I guess I can live with std::string, though I'm a stickler for small file size. However, in this case I s'pose the sacrifice is far worth it.
Quote:Original post by Emmanuel Deloget
On a side note, your code is rather dangerous: what if getWindow() returns NULL? What if editFocus is greater than the size of the edit box vector? You get it: your code will painfully die, and the world will collapse in a big crunch. To be more precise, code that looks like a->b->c should be avoided.

My code has built in error-handling. Like with getWindow, it checks if the window is NULL before returning and, if so prints out an error for the user. This way in the future when I redistribute this to others, if a bug comes across they can send it to me along with printed out info and I can work on fixing it.

Anywho, thanks again!

--Aternus
Quote:Original post by Aternus
Alright, thanks a bunch. I guess I can live with std::string, though I'm a stickler for small file size. However, in this case I s'pose the sacrifice is far worth it.
Quote:Original post by Emmanuel Deloget
On a side note, your code is rather dangerous: what if getWindow() returns NULL? What if editFocus is greater than the size of the edit box vector? You get it: your code will painfully die, and the world will collapse in a big crunch. To be more precise, code that looks like a->b->c should be avoided.

My code has built in error-handling. Like with getWindow, it checks if the window is NULL before returning and, if so prints out an error for the user. This way in the future when I redistribute this to others, if a bug comes across they can send it to me along with printed out info and I can work on fixing it.

Anywho, thanks again!

--Aternus

It is a good idea, but it is still not the good way to do. First, it does not prevent the bug at all, meaning that you are going to ship a product which might be full of bugs. When you buy a game, you don't care about the error messages when your game crashed. Even if you care, it still mean that your game crashed, and you wasted your money on a product that don't work.

Your method also assume that your user will send you the bug report - instead of deleting the program (and, if you are lucky, send you a flaming email). How many time do you think that users take the time to send a bug report to the program maintainer? You'll be surprised to know that only a small percentage of them actually do this. For example, I'd rather remove the program and use another one instead of hoping that bug will be corrected by the program developer.

Moreover, from a code point of view, in most cases the callee is not supposed to verify the value it returns - the caller must do it because he is the only one who will know what to do with the value. For example, let's think to this case:
Window w* = getWindow();
if (w == NULL) {
do something special
}Why do you want this code to print an error?

Bugs cost a lot of money. When you can avoid a bug, do it - don't wait for it to happen. Error messages might be good to explain the cause of a bug, but what would be even better is to not have any bug at all :)

Regards,

This topic is closed to new replies.

Advertisement