Thread: C gurus ans this?

I need to return a char pointer from func.
The function takes a string and then always returns a 40 char value as a result of calling "giveout". I want to optimize this code in efficient way and want to use malloc and free to aviod any mem leak issue.

Code:

char * retrieve ( char *para1)
{
}

Since your malloc is a fixed (and small size), and you don't even return it, why bother?

char outputstr[41];
would be more efficient

41? Yes, you forgot to count the \0 at the end.

You also forgot the \0 at the end of str[2] as well.

There are a few holes, or should I say undocumented "assumptions", in your logic.

1) You don't verify if inputstr will hold your "doubled-in-length" answer
2) There's really no point to return a pointer to the same storage location you were passed
3) There is no warning or error when your malloc fails, therefore, your answer will not be converted
4) Depending on the compiler, this:

Code:

if ( outputstr != NULL )

might be better written as this:

Code:

if ( outputstr )

5) You really don't need the err variable
6) You could initialize RC=0 during the compile instead of at runtime
7) You could calloc() the area for outputstr instead of using malloc() and then clearing it manually
8) You could actually get rid of variable rc since you are not using it for anything

Originally Posted by Jenny

1) Are there any problems with your code?
>> No , there is no problem in current code but I want to optimize this code in efficient way and want to use malloc and free to avoid any mem leak issue.

You keep saying that but it keeps not making sense. Using malloc when you don't need to (as is the case here) if anything can introduce the possibility of a memory leak. Not using it removes that possibility, and improves speed also.

2) Is there a reason you are using the size of a char pointer and multiplying it by 40?
>> The function takes a string and then always returns a 40 char value as a result of calling "giveout" func hence I multiplying it by 40.

Yes but you're not returning 40 strings, you're returning a string of length 40, except that you're allocating 160 bytes to do so.

Looking forward for your help in optimizing the code.

Why would it need to be any faster? Sure there is plenty of unnecessary junk such as zeroing the first 40 bytes of the buffer and then overwriting that, but it needs to be correct first. At the moment a memory allocation failure would silently go unnoticed, plus there is a buffer overflow in the "str" array.

Thanks Sean for your reply.

Originally Posted by sean

Your function takes a pointer as an argument, and that same pointer is returned. Even though the actual data stored in memory at that location (i.e. the string itself) is modified, the location you're pointing to is still the same location.
[/code]

So you recommend that I should change the below form

Code:

char * retrieve ( char *inputstr )
{
   .
   .
   .

}

to ?

Can you tell me what I should change it to?

>> Can you tell me what I should change it to?
Well you don't need to change it to anything. Returning a pointer that was passed as a parameter is actually kind of common (see strcpy() for instance). But if you didn't want this redundancy, you could change your function to not return anything at all. ie:

Code:

void retrieve (char *inputstr)

Originally Posted by Jenny

Can you explain in details what you mean here?

Hi Jenny.

In the original code you posted for your function, (you've since edited it out), you were converting from hex to printable hex, therefore, the end result is going to be twice as long when you are done with it.

For example. if the input value is X'012345', which is 3 bytes, the output answer will be '012345', which is 6 bytes.

My point was that you need to be certain that the storage pointed to by inputstr is large enough to hold double its initial value.

Dino