Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Remove invoking of the non-trivial destructor. #128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove invoking of the non-trivial destructor. #128

wants to merge 2 commits into from

Conversation

sabbakumov
Copy link

Don't expose the risk of accessing objects after the end of their lifetime

Don't expose the risk of accessing objects after the end of their lifetime
@artwyman
Copy link
Contributor

I can see the purpose for this, and have worked in codebases where we would've made this tradeoff. Trading deinit-time safety for a resource leak feels like a trade-off which is situation-specific, though. E.g. in an app which needed to repeatedly load and unload a dynamic library this kind of memory leak can become meaningful, and even before that it can trigger warnings in people's toolchains. So, I don't think this is a bad idea, but I'm not sure about merging it into the standard json11 implementation.

@sabbakumov
Copy link
Author

sabbakumov commented Oct 22, 2018

What do you think of the Chromium's NoDestructor class? It doesn't allocate new memory on library loading/unloading.
But tools still can signal a memory leak warning. However, the memory blocks are still reachable, so I think it's a minor issue.
This class is highly used inside Chromium.
I've added this class to the Pull Request.

Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this approach has the same behavior and concerns as your original approach. The T in this case is now a static allocation rather than an allocated block which is never freed, but all the allocations held inside of that T would be allocated and not freed. As I said before, I think this is probably the right approach for many use cases, but I don't think it's universal enough to be the only one in this library. One possible compromise might be a #ifdef to allow for destructors to be skipped only when explicitly requested.

Also taking this code from Chromium probably comes with a need to add some sort of license acknowledgement. That seems like a lot of logistical overhead for a very simple piece of functionality, as does the amount of code involved here, so I think special-purpose code is better than a general utility class here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants