Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update deconstructor(chain) in all projects #101

Open
deccer opened this issue Mar 29, 2022 · 4 comments
Open

Update deconstructor(chain) in all projects #101

deccer opened this issue Mar 29, 2022 · 4 comments
Labels
Milestone

Comments

@deccer
Copy link
Contributor

deccer commented Mar 29, 2022

DearImGuiApplication::~DearImGuiApplication()
{
    ...
    Application::Cleanup();
}

Application::~Application()
{
    Application::Cleanup();
}

void Application::Cleanup()
{
    glfwDestroyWindow(_window);
    glfwTerminate();
}

looks fishy

@deccer deccer added the good first issue Good for newcomers label Mar 29, 2022
@deccer deccer added this to the Part 1 milestone Mar 29, 2022
@deccer deccer changed the title Project: Check deconstructor(chain) in all projects Check deconstructor(chain) in all projects Mar 29, 2022
@JuanDiegoMontoya
Copy link

JuanDiegoMontoya commented Mar 29, 2022

This destructor should be sufficient for the derived application classes:

DearImGuiApplication::~DearImGuiApplication()
{
    _deviceContext->Flush();
#if !defined(NDEBUG)
    _debug->ReportLiveDeviceObjects(D3D11_RLDO_FLAGS::D3D11_RLDO_DETAIL);
#endif
}

I removed all _member.Reset(); calls and the Application::Cleanup(); call at the end (the base class calls it already in its destructor).

@spnda
Copy link
Member

spnda commented Mar 29, 2022

I personally think Cleanup should be a protected member function but not be called from the destructor, instead of being private, which would also be an alternative. Same way a constructor doesn't automatically call Initialize; the caller might want to manage resources differently and keep the Application object alive but already call Destroy or Cleanup. In that case, it's might be beneficial for the destructor to call Cleanup only if the caller hasn't done it already, or call it automatically at the end of the Run function. Another idea would be to use a syntax similar to this entirely:

ExampleApplication app("Example");
app.Init();
app.Run();
app.Destroy();

This does add a bit of boilerplate code but it ultimately gives a bit more control and can be a good concept to deliver to readers who are not yet familiar with C++ or programming in general. In my own application I even give the caller the responsibility of handling the frame loop and calling rapi->draw() each frame.

@deccer
Copy link
Contributor Author

deccer commented Mar 29, 2022

I believe that was my initial intention, to call Cleanup in App::Run() after the gameloop, it is protected already (imho) (if not then yes it should be protected virtual)

That would render the dtor useless and we could technically remove it - unless c++ works differently again

@spnda
Copy link
Member

spnda commented Mar 29, 2022

The destructor can be simply replaced with the following, as unique_ptrs and ComPtrs will automatically destroy their underlying object in the default destructor.

virtual ~Application() = default;

~Derived() = default;

Marking the ~Application virtual will also allow any derived class to override it and implement a custom destructor, if required.

@deccer deccer changed the title Check deconstructor(chain) in all projects Update deconstructor(chain) in all projects Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants