free not working with malloc

In trying to get the text from a list box, concatenate that with the url to dictionary.com, then look up the word useing ShellExecute, the following didn't go according to plan. It works perfectly the first time, but the second time URLbuf gets merged with the previous text. eg. looking up "emitt" -> URLbuf = "http://dictionary.reference.com/browse/emitt". But then trying to look up "emitter" -> URLbuf = "http://dictionary.reference.com/browse/emittemitter". Here is the code, with 2010 being the ID of the listbox:

Code:

---

char *URLbuf = (char*) malloc(50);
BOOL lResult = SendMessage(GetDlgItem(hwnd, 2010),LB_GETCURSEL,0,0);
if (lResult >= 0) {
        SendMessage(GetDlgItem(hwnd, 2010),393,lResult,(WPARAM)URLbuf);
        URLbuf = strncat("http://dictionary.reference.com/browse/", URLbuf,50);
        ShellExecute(hwnd,"open",URLbuf, NULL, NULL, SW_SHOWNORMAL); }
        free(URLbuf);
        return 0;
        break;
}

---

I have tryed setting *URLbuf = 0 or setting it to NULL, but that disables URLbuf from holding anymore strings. If anyone could effectively set URLbuf = "" so each time the buton is clicked and this code is executed, I wouldn't have to restart the application. BTW: this is for a target solver for the SMH (my second GUI application fully coded in C).

You are modifying a string literal. I'm surprised Windows lets you do that.

Also, strncat returns the destination string, so your URLbuf is no longer pointing to your allocated memory. Calling free at that point is bad news.

try using sprintf() instead of strncat, it prints directly to the supplied buffer and doesnt move it

I'm confused by that code...

Quote:

---

Originally Posted by P4R4N01D

Code:

---

char *URLbuf = (char*) malloc(50);
BOOL lResult = SendMessage(GetDlgItem(hwnd, 2010),LB_GETCURSEL,0,0);
if (lResult >= 0) {
        SendMessage(GetDlgItem(hwnd, 2010),393,lResult,(WPARAM)URLbuf);
        URLbuf = strncat("http://dictionary.reference.com/browse/", URLbuf,50);
        ShellExecute(hwnd,"open",URLbuf, NULL, NULL, SW_SHOWNORMAL);
}

free(URLbuf);
return 0;
break;
}

---

---

I assume it's supposed to look something like that. But then again, if so, then what the heck are break and the last } doing there???
You haven't indented properly, I take it? That's my only guess.

Quote:

---

Originally Posted by Elysia

I assume it's supposed to look something like that. But then again, if so, then what the heck are break and the last } doing there???
You haven't indented properly, I take it? That's my only guess.

---

Perhaps its inside a switch statement [yes, it's superfluous in this case, as there is a return as well, but it's not a bad idea to have "extra" break in a switch run, just to make it symmetrical - as long as the compiler doesn't complain about "unreachable code" that is].

--
Mats

return will return control to the caller immediately, execution will never reach the final break.

here's an example of what you're trying to do, although it could probably be written alot better, it's just an example:

Code:

---

char *URLbuf, *url = "http://dictionary.reference.com/browse/", buf[50];

BOOL lResult = SendMessage(GetDlgItem(hwnd, 2010),LB_GETCURSEL,0,0);
if (lResult >= 0) {
        SendMessage(GetDlgItem(hwnd, 2010),393,sizeof buf,(LPARAM)buf);
                URLbuf = malloc(strlen(url) + strlen(buf) + 1);
                strcpy(URLbuf, url);
                strcat(URLbuf, buf);
        ShellExecute(hwnd,"open",URLbuf, NULL, NULL, SW_SHOWNORMAL);
                free(URLbuf);
}

return 0;

---

I always put the breaks in, even if they are not required.

I do so because I assume I am not the only person whom will read the code (so I try to make it as comprehendable as possible).

Quote:

---

Originally Posted by novacain

I always put the breaks in, even if they are not required.

I do so because I assume I am not the only person whom will read the code (so I try to make it as comprehendable as possible).

---

And of course, that is two-sided coin. Someone else will read the code and think "Why is there a break there" - but I agree with that method. Belts and braces approach is always better than sloppy loose style. Consistancy is one of the key points in programming. If you follow the same pattern for doing similar things in different places, then you end up with a better overall code that most people will be happy to read, modify and debug.

Using different styles, different ways of doing similar things for each case, etc, etc, you get a mess that looks like a plate of spaggetti ravaged by a bull-dog, and no one will touch it with a barge-pole unless they HAVE to.

What pattern you follow is a bit less important, consistancy is more so. Make sure variables are spelled the same way every time.

It's a good idea to not use "no" as a short for of "number", "n" or "num" or "numberOf" are much better options [and decide WHICH of those you should use, and do that EVERY time]. "noSomething" can be misinterpreted to mean "I don't want Something" when I call the function, rather than "number of somethings".

Of course, it's always hard to get consistancy when there are dozens of programmers that have different ideas of how to do things right. This is when coding standards come in handy. Some people will HATE to have the braces on the next line after if, whilst others will not appreciate the idea of what spaces you use - but it's important that it's the same all over the code, or you get confusing messages betwen different bits of code.

--
Mats

To clear a few things up, yes it is inside a switch statement (WM_COMMAND). the if statement should read: if (lResult > 0). It doesn't make sense to have a word looked up if it is of 0 length. Also thanks for noticing that the return 0 is not needed.

Replying to sl34k's post, this code doesn't even work the first time, as buf doesn't get appended to URLbuf, so ShellExecute opens up the browser to "http://dictionary.reference.com/browse/".

OK I finally solved it. Thanks very much to sl34k and the idea of using strcopy (would never have thought of it). The third paramater to SendMessage is the index of the item to get not the length of it.

Code:

---

char *URLbuf, *url = "http://dictionary.reference.com/browse/", buf[10];
        BOOL lResult = SendMessage(GetDlgItem(hwnd, 2010),LB_GETCURSEL,0,0);
        if (lResult >= 0) {
                    SendMessage(GetDlgItem(hwnd, 2010),393,lResult,(LPARAM)buf);
                    URLbuf = malloc(strlen(url) + strlen(buf) + 1);
                    strcpy(URLbuf, url);
                    strcat(URLbuf, buf);
                    ShellExecute(hwnd,"open",URLbuf, NULL, NULL, SW_SHOWNORMAL);
                    free(URLbuf);
        }
        break;

---