-2

Since I am a newbie, I just want to ask more experienced programmers in C++ is this a good C++ practice. I have a function which returns *char. This function is in dll.

I have defined global *char variable, and that is what is returned. I am using libcurl for POST request method. Since every response is in different length I am using new. When you are using new, it also necessary to use delete, and I want to check am I using delete and pointer correctly?

char *response

struct MemoryStruct {
char *memory;
size_t size;
};

char *function() {
    //...
    // libcurl code
    //...
    
    // is this part of a code a good practice?
    if (response == nulptr) {
        response = new char[cunk.size];
        memcpy(response, chunk.memory, chunk.size);
    } else {
        delete[] response;
        response = new char[chunk.size];
        memcpy(response, chunk.memory, chunk.size);
    }
    
    //...
    // libcurl cleanup
    //...
    
    return response;
}

Is this OK or is there another (better) way to do this?

Thank you for any help.

EDIT: I forgot to mention that result is not returned to C++, it is returned to Clarion. So I cannot use smart pointers or string.

look_up
  • 41
  • 8
  • 3
    "Good practice" would be to use a wrapper such as `std::string` to handle memory management. Why exactly do you need to work with raw `char*`? – UnholySheep Nov 06 '21 at 12:21
  • 1
    You don't need the `if` at all. Just `delete[] response;` always. It is okay to `delete[]` a null pointer. (But really, you should use a `std::unique_ptr`.) – Raymond Chen Nov 06 '21 at 12:21
  • There's no clear reason why `response` is a global. It would seem that the value returned from `function()` and the value stored in `response` are the same? Other code can look at either one and ignore the other? – Drew Dormann Nov 06 '21 at 12:40

2 Answers2

2

No, this is not good C++ practice.

The usual convention is to never use raw pointers for ownership. Meaning that both response and the return type should be std::unique_ptr.

What is the point of global response? So that the user does not have to delete the memory, right? That's solved by the paragraph above. Be very careful about using these hidden globals, the function is no longer re-entrant and not thread-safe. What would happen to response and return values for two parallel calls of func? Certainly nothing good.

Even POSIX moves away from functions that have inner hidden persistent state like strtok and replaces them with strtok_r safe versions.

Quimby
  • 17,735
  • 4
  • 35
  • 55
2

It should be something as simple as this:

  • no new/delete
  • no memcpy
  • no global variables
#include <string>
#include <iostream>

std::string function()
{
    // libcurl stuff
    const char* chunk{ "Hello world!" };

    std::string response{ chunk };
    return response;
}

int main()
{
    auto response = function();
    std::cout << response;
    return 0;
}
Pepijn Kramer
  • 9,356
  • 2
  • 8
  • 19