2

I am working on some legacy code where I have to make some changes in the cpp file.The cpp file contains entire code in extern "c" block -

I updated a function that returns a char* .The code looks something like func1() below. Since I use std::strring and stringstream I included the sstream and string header files before extern block. The below function is called from both c and cpp files.So I cannot return std::string here -

char* func1(someStruct* pt){
    std::strig nam = somefunc(pt);
    //have to append some integer in particular format
    std::stringstream ss;
    ss<<nam<<pt->int1 ......;

    nam = ss.str(); 
    //More code here for returning char* based on queries - (a)
}

At one of the places where this function is called -

void otherFunc(.....){
    //......
    char* x = func(myptr);
    if(based_on_some_condition){
        char* temp = func3(x); //returns a char* to dynamically allocated array.
        strcpy(x,temp);       //copying (b)

    }
    //..........
}

Following is my query -
1) At (a) I can return char* in following 2 forms.I have to make a decision such that copying at (b) does not cause any undefined behavior -

i)Create a char array dynamically with size = nam.length()+10 (extra 10 for some work happening in func3).<br>
    char* rtvalue = (char*)calloc(sizeof(char),nam.length()+10);
    strcpy(rtvalue,nam.c_str());
    return rtvalue;
    And free(temp); in otherFunc() after strcpy(x,temp);

ii) Declare 'nam' as static std::string nam;
    and simply return const_cast<char*>(nam.c_str());
    Will defining 'nam' with static scope ensure that a correct return happen from function (ie no dangling pointer at 'x')?
    More importantly, can I do this without worrying about modification happening at (b).

Which one is a better solution?

Anil
  • 111
  • 6
  • 3
    Solution 1 is inefficient and Solution 2 won't work because the caller will call free(). Have a look at strdup. – rustyx Jun 08 '18 at 17:27
  • 1
    I think Creating a dynamic array as explained in (i) would be better as you no longer will be affecting the data associated with string 'nam' . All changes will happen on a copy – abhishek gupta Jun 08 '18 at 17:28
  • @rustyx free() is part of option i) – Anil Jun 08 '18 at 17:33
  • For solution 2, even better: `return str.data()` - no const-cast – SergeyA Jun 08 '18 at 17:46
  • @SergeyA For str.data() - There are no guarantees that a null character terminates the character sequence pointed by the value returned by this function – Anil Jun 08 '18 at 17:58
  • @BigA, this is not true since C++11 (7 years ago!): http://en.cppreference.com/w/cpp/string/basic_string/data – SergeyA Jun 08 '18 at 18:04
  • I would be tempted to make the caller allocate the buffer and pass it to your `func()` and `func3()` functions. Otherwise ownership looks a bit messy. Which function allocates what? – Galik Jun 08 '18 at 18:05
  • 1
    What is the meaning of that pointer that is returned? I guess it is supposed to point to a C-style string, right? In that case, it could probably be `const`, but that wouldn't help much. Rather, the question is who owns that memory and from that follows who has to release that memory after use. From the answer to these questions follows the requirements on the chosen implementation, be it in C or C++. Start with these, so that you can finish the interface description. – Ulrich Eckhardt Jun 08 '18 at 18:11

2 Answers2

1

Problem is returning a char *. When you using C++ you should not use this type. This is not C! std::string or std::vector<char> should be used.

If you will use char * as return type in this kind of function it will end with undefined behavior (access to released memory) or memory leak.

If you will use static std::string nam; function will maintain internal state and this is always leads to trouble. For example if you create threading functionality you will have undefined behavior. Even worse if you will use this function twice for some reason result of second call will have impact on result for first call (for example your coworker will use this function since he will not expect hiden side effects).

If you are designing some API which should be accessible from C code than you should design this API in different way. I do not know what kind of functionality you are providing by most probably you should something like this:

char *func1(someStruct* pt, char *result, int size){ // good name could be like this: appendStructDescription
    std::strig nam = somefunc(pt);
    //have to append some integer in particular format
    std::stringstream ss;
    ss<<nam<<pt->int1 ......;

    nam = ss.str(); 

    int resultSize = std::min(size - 1, nam.length());
    memcpy(result, nam.c_str(), resultSize);
    result[resultSize] = 0;
    return result + resultSize;
}

This approach has big advantages: responsibility for a memory management goes to caller, user of the API understands what is expected.

Marek R
  • 32,568
  • 6
  • 55
  • 140
  • As mentioned in the question func1() will be used in some c code as well. So, returning std::string is not an option – Anil Jun 08 '18 at 17:59
0

It is true that you should return string, but if you absolutely need to return char*, first method is better. And don't forget free. Otherwise, expressions like strcmp(f(pt1), f(pt2)) would return unpredictable results.

user31264
  • 6,557
  • 3
  • 26
  • 40