0

I have a library which the structure is something like this:

class Subfeature {
};

class BigFeature {
  Subfeature* subfeature;
  // more properties/methods ...

public:
  Subfeature* get_subfeature();
  BigFeature();
  ~BigFeature(); // In this destructor, subfeature is deleted
};

I found out that this library is very messy (but it is so large for me to change something), and so I decided to make a wrapper class with following structure:

class BigFeatureWrapper {

  class SubfeatureWrapper {
    Subfeature* subfeature;
    // more properties/methods etc.

  public:
    // This constructor calls get_subfeature method in BigFeature and set it in subfeature property
    SubfeatureWrapper(BigFeature* bigfeature) {
      this->subfeature = bigfeature->get_subfeature();
    };
  };
  
  SubfeatureWrapper* subfeaturewrapper;
  BigFeature* bigfeature;

public:
  BigFeatureWrapper() {
    this->bigfeature = new BigFeature();
    this->subfeaturewrapper = new SubfeatureWrapper(this->bigfeature);
  }
  ~BigFeatureWrapper() {
    delete this->bigfeature;
    // Should I define destructor for SubfeatureWrapper
    // and write `delete this->subfeaturewrapper;` here?
  }
};

Here, should I define and destruct subfeaturewrapper to prevent undefined behaviour, memory leak etc.?
Because the destructor of BigFeature deletes the pointer for subfeature,
I'm wondering if I should remove Subfeature* subfeature inside SubfeatureWrapper also (or could it be redundant).

I'm completely new to pointers in C++C/C++, so detailed explanation will really help. Thank you!

P.S. I know the existence of smart pointers, but I can do nothing about it because my [external] library uses raw pointers :(

Cythonista
  • 91
  • 7
  • 4
    Rule of thumb: every `new` should have exactly one corresponding `delete`. Can you answer your own question: will your code execute exactly one `delete` for each `new`? – Sam Varshavchik Jul 21 '23 at 13:29
  • 2
    There is no such thing as "C/C++". These are different languages with different rules. – Evg Jul 21 '23 at 13:31
  • 4
    Another important point is wording: "delete" does not delete pointers, but it deletes objects. – gerum Jul 21 '23 at 13:31
  • 3
    *I'm completely new to pointers in [C++]...* You should use `std::unique_ptr` or containers like `std::vector`. Don't use `new` until you have several years of experience with C++. – Eljay Jul 21 '23 at 13:36
  • 1
    *but I can do nothing about it because my library uses* -- If it's your library, then you can do something about it -- change it to use smart pointers. – PaulMcKenzie Jul 21 '23 at 13:49
  • Thank you for all your comments! I'm assuming, then, that I need to add a destructor for subfeaturewrapper but I shouldn't delete subfeature inside subfeaturewrapper, would that be correct? – Cythonista Jul 21 '23 at 13:50
  • 1
    @Cythonista Given the code you posted, your classes violate the [rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three), so right from the start the classes are flawed. Go to the **Managing Resources** section, and look at the similarities between the example and your code. The bottom line is that you can't simply throw `new` and `delete` around in code without knowing what you're doing, and all the implications of using `new/delete`. – PaulMcKenzie Jul 21 '23 at 13:52
  • `subfeaturewrapper` probably does not need to be a pointer at all, as it only exists inside of `bigfeaturewrapper` and shares its lifetime. – nick Jul 21 '23 at 13:58
  • 4
    @Cythonista *but I can do nothing about it because my [external] library uses raw pointers :(* -- Since you are create a wrapper class, just because what you are wrapping uses raw pointers doesn't mean the wrapper itself must use raw pointers. The whole idea of `std::unique_ptr` is to take a "raw pointer" that already exists, and manage it properly. If there is code that requires the raw pointer, then there is the `get()` member function for `std::unique_ptr` (and `std::shared_ptr`) that returns the raw pointer. – PaulMcKenzie Jul 21 '23 at 13:59
  • 1
    *"`// This constructor calls [...]`"* -- Don't just tell us what the constructor does. Show it. You've given inline, simplified definitions for other functions, so why not this one? `SubfeatureWrapper(BigFeature* bigfeature) : subfeature(bigfeature->get_subfeature()) {}` (or however you have it implemented) – JaMiT Jul 21 '23 at 14:00
  • Thank you! Didn't know unique_ptr could wrap raw pointers, will definitely check one. Also, I will go sleep immediately, so I may respond late for comments after now. Thank you very much – Cythonista Jul 21 '23 at 14:03
  • You don't need dynamic memory allocation to use raw pointers in a library. – jabaa Jul 21 '23 at 15:32
  • 1
    Not sure why you need dynamic allocations here at all. If you move the `SubfeatureWrapper` to namespace scope, you could just implement `BigFeatureWrapper` as `class BigFeatureWrapper { BigFeature bigfeature; SubfeatureWrapper subfeaturewrapper {&bigfeature}; public: };` and wouldn't even need to create constructor/destructor manually... – fabian Jul 21 '23 at 16:15
  • Does ↑ means that I can use only one pointer to bigfeature, which will also do the destruction of subfeature responsibly? – Cythonista Jul 22 '23 at 05:22

0 Answers0