Handling Destructors properly in header?

[i773]

I'm coming from a background in C# and I am dealing with deconstructs right now in c++ and the way that I created my functions I think I could have done better:

class MyPlugin{
public: 
  void SomeFunction(); 

  static char* j;
  static char* AppendExtension(string value, string s = ".png")
  {
    value.append(s);
    j = new char[strlen(value.c_str()) + 1];
    strcpy(j, value.c_str());
    return j;
  }
}

My question is, at first I had j in the scope of AppendExtension but if I need to release the new memory (j) I have to keep it in the scope of the class. Now, I see that I might be creating my headers wrong:

1. How would I best create a deconstructor for releasing (j)?
2. Is the way I am constructing my classes bad practice? Should I be keeping all my logic inside the source (I like the idea of keeping logic that is used like a structure inside the header)

[jonnin]

why are you using a mix of C++ and c style strings? prefer
string a;
string b;
a = b; //prefer to strcpy etc

that aside

the deconstructor for MyPlugin can release j, but any pointer that copied j would also be invalidated. If that is a problem, you need a more complicated solution with reference counting and such. Or just make sure you don't hold onto it externally.


It depends on what you put in a header file whether its OK, violation of general conventions, or potentially a problem for your code if you do it. The first 2 compile and work fine. The last one may compile and work until it doesn't.... !

[keskiverto]

What is that code trying to achieve? Once we understand the what, we can start to think about "how in C++".

Why both strings (std::string) and char arrays?

[i773]

"why are you using a mix of C++ and c style strings?" Managed code from C#.

"the deconstructor for MyPlugin can release j," Would I not want to have a function deconstructor for this? Would it look like this:

~AppendExtension(){
    delete j;
}

"It depends on what you put in a header file whether its OK," Is the way I am doing it okay to use is it a practiced example.

Thanks for your reply!

"What is that code trying to achieve? Once we understand the what, we can start to think about "how in C++"." The code itself is just used as an example to show how I am constructing the header, and if I should be creating it like so in c++, but if you need to know it's returning a char array to a photoshop plugin (That only accepts that) from a c# application.

[keskiverto]

What does the photoshop plugin do with a char pointer?
How long does the array have to exists?

Do you realize what happens, if the user does:

char * a = MyPlugin::AppendExtension( "foo" );
char * b = MyPlugin::AppendExtension( "bar" );
// use a 

The j and AppendExtension are static members. They do not need any MyPlugin object. Lets assume that you do call delete[] in destructor.

int main() {
  MyPlugin x;
  MyPlugin y;
  // double deletion of what was never allocated?
}

The main question: What in header, what not?

The class is defined in the header. The header might be included into multiple translation units. Inlined member function implementations can be there, but the main gist is declaring the interface of the class for the users. Look up Herb Sutter and pimpl idiom about hiding the private parts of a class from the users.

Static member variables cannot be initialized in the header. If the header is included in more than one translation unit, then the definition of the static member will be defined more than once, which is a linker-stage error.

The implementation of the destructor may not always be inlined.


Lets assume that the header is a part of a library. Some programs use and dynamically link the library. Implementation of a member is in the header. You find and fix a bug in the function. The interface does not change. You obviously recompile the library. Every program has to recompile too, for their sources include the header.

Plan B: the implementation is in the source. You fix and recompile the library. Distribute it to user. Done. Programs do not have to recompile, for dynamic linker can link to the new copy too.

[i773]

"What does the photoshop plugin do with a char pointer?" Uses it for its own application functions, [opening docs, saving files, etc]

How long does the array have to exists? one function call:

MyOtherPluginClass::OtherFunction(){
char* j = MyPlugin::AppendExtension(buffer);
MyPlugin::OpenDoc(j);
}

"Do you realize what happens, if the user does:" Sorry I don't understand, are you asking me if I know what is returned from AppendExtension? I wrote that function, so yes.. I might be missing something else you're trying to help me understand the question is a little confusing for me.

"The j and AppendExtension are static members." Yes I need to access them from external classes.

My original code was:

  static char* AppendExtension(string value, string s = ".png")
  {
    value.append(s);
    static char* j = new char[strlen(value.c_str()) + 1];
    strcpy(j, value.c_str());
    return j;
  }

Would I still need to release that?

[jonnin]

yes, this looks almost fine. delete deletes 1 item, unless you add [] to it. You allocated more than 1 character, so you need the [] to tell it to delete it all. Also, since it is static, you better set it to null.

~AppendExtension(){
delete [] j;
j = nullptr;
}

Headers should not contain code unless necessary (templates are a whole new ballgame here). It is seldom necessary outside of templates.

and, just to be clear... you want to be careful about this...


char * cp;

{
MyPlugin p;
cp = p.AppendExtension;
} //scope ends, p is destroyed!!

...
cp[0] = '!'; //oops, cp was destroyed too!

and you also want to be careful about that static keyword. Watch this one:

MyPlugin q;
{
MyPlugin p;
} //p is destroyed, so is Q's j pointer!!

so if you have more than 1 copy of this class, your code will be "fragile".
If you try to catch handles to j outside the class, your code will be "fragile".

[jlb]

Why can't you just use the c_str() of the string instead of trying to manually allocate memory?

And remember that in C++ not everything must be part of a class. It is perfectly acceptable to have variables and functions that are not part of any class.

[i773]

yes, this looks almost fine. delete deletes 1 item, unless you add [] to it. You allocated more than 1 character, so you need the [] to tell it to delete it all.

Why would I be getting a invalid deconstruct declaration? I'm placing it in the scope of my class

  static char* AppendExtension(string value, string s = ".png")
  {
    value.append(s);
    char *j = new char[strlen(value.c_str()) + 1];
    strcpy(j, value.c_str());
    return j;
  }

  ~AppendExtension() {
    delete[] j;
    j = nullptr;
  }

"Why can't you just use the c_str() of the string instead of trying to manually allocate memory?"

I did try that but: "argument of type "const char*" is incompatable with parameter of type "char*"

Thanks guys!

[jlb]

I did try that but: "argument of type "const char*" is incompatable with parameter of type "char*"

Where did this problem occur? Show the code where you tried using the c_str() and post the complete error message as well.

[jlb]

Why would I be getting a invalid deconstruct declaration? I'm placing it in the scope of my class

What I see is a memory leak if you call this function more than once since you overwrite the pointer each time you call the function.

[i773]

I'm mapping a function to a string and invoking it. The code won't be much help, so I'll just give you the actual photoshop function with a string to c_str()

https://preview.ibb.co/nj6NAQ/Screenshot_83.png

SPAPI OSErr (*PutString)(PIActionDescriptor descriptor, DescriptorKeyID key, char* cstrValue);

No overloads just that one function.

"What I see is a memory leak if you call this function more than once since you overwrite the pointer each time you call the function." yes, I am calling it a few times, so this is the slightly confusing part to me, how can I release it when I still need to return it?

[jlb]

Is the cstrValue parameter changed after the function call?

I'd think about having a fairly large buffer that you can use for this function. Something like:

char buffer[1000];
strcpy(buffer, yourString.c_str());
// Call your function here.
// After the function call you can reuse buffer if you so desire. 

[i773]

"Is the cstrValue parameter changed after the function call?" I don't hold onto it outside the one function call, no.

Yeah, I can use this, not a huge deal I guess I don't need the exact size for the buffer:

  static char* AppendExtension(string value, string s = ".png")
  {
    char buffer[256] = "";
    strcpy(buffer, value.c_str());
    return buffer;
  }

Should I be assigning it to "" on construction like this?

Thanks!

[dhayden]

Use string so you don't have to worry so much about the memory management. Then you won't even need a class.

string value;
string valueWithExtension = value + ".png";  // easy peasy! 

If that function really REALLY needs a non-const char*, then create it and destroy it right around the call rather than leaving the memory management it up to some distant class that will corrupt memory if it isn't used exactly right.

char *tmpStr = strdup(valueWithExtension.c_str());
error = sPSActionDescriptor->PutString(EPSDescriptor, "ovFN", tmpStr);
free(tmpStr);

[keskiverto]

Is the cstrValue simply an input parameter?
Does the function promise to not modify the string?
Does the function promise to not take ownership of the memory?

If yes and you are using C++11, then const_cast might be possible.

MyOtherPluginClass::OtherFunction() {
  char* j = MyPlugin::AppendExtension(buffer);
  MyPlugin::OpenDoc(j);
}

No MyPlugin object was created or destroyed in that code.

MyOtherPluginClass::OtherFunction() {
  std::string j = buffer + ".png";
  OpenDoc( j );
  // j destructs here
}

A static member can be public, protected or private. Being static is less about accessibility.
A static member variable is similar to a singleton global variable. Neither singletons nor globals are fun any more.


Edit: More fun with interfaces: http://www.fluentcpp.com/2017/06/20/interface-principle-cpp/