Make sure all singletons are set to null and make env a global ptr #44
Conversation
|
|
||
| static environment& inst() | ||
| { | ||
| static environment env; |
There was a problem hiding this comment.
Normally this is the comon way to create a singleton and is free of memory leeks. Is there a reason why you have changed it?
This version is also race condition free and guarantee that there exists only one version of this class.
There was a problem hiding this comment.
This is because you are creating a copy of the environment object in DLLs. If you create a singleton after you create a DLL copy you will have a duplicate.
There was a problem hiding this comment.
@psychocrypt Can you let me know if you approve or not here?
|
I approved this PR but can't test it currently in windows. Do you tested it in windows? |
|
There is no OS-specific code in that PR - merging. |
|
note: all the singleton registering in one instance is only made for
windows. Shared library handling dis different in windows and linux :-(
|
|
What's the difference? There is none from what I can see in the code. |
|
In the current implementation there is no difference. But the reason why we need to path the singleton class environment to the dll/so is that a singleton under windows is only unique within a shared libarary. This means if the main binary create a singleton the singleton is not visible in the dll. In linux a singleton is which is created in the shared library or in the main binary is unique ofer all shared libraries and the main binary. |
|
You mean the difference in symbol visibility between Windows and Linux? If not then please link to the MSDN article. |
Yes, for me as linux developer this was absolutely new, that a singleton is not unique because of the difference in the symbol export handling. Sry I can't point to the sources I worked on this during the refactoring and have not stored it :-( note to myself: bookmark links |
| executor* pExecutor; | ||
| params* pParams; | ||
|
|
||
| printer* pPrinter = nullptr; |
There was a problem hiding this comment.
@fireice-uk Could you please point me to a reference which describe why it is allowed to initialize member out of the constructor. Is this because of initializer lists?
There was a problem hiding this comment.
There was a problem hiding this comment.
That's correct. For C++11 both those styles are equivalent. I chose the second one because it is clearer and nullptr is a constexpr.
| if(env == nullptr) | ||
| { | ||
| if(init == nullptr) | ||
| env = new environment; |
There was a problem hiding this comment.
@fireice-uk Could it be that this is the reason for #51. That environment is handled as POD and the pointer will not be initilized with a nullptr?
There was a problem hiding this comment.
There was a problem hiding this comment.
If this can result in a uninitialized type than we need also check all other singletons.
This change makes sure of two things:
Not testes at all, but should work. Let me know if there are any problems