Make MMapFactory and VMapFactory singleton creation thread safe indep… #19070
Conversation
| g_MMapManager = NULL; | ||
| } | ||
| static MMapManager g_MMapManager; | ||
| return &g_MMapManager; | ||
| } | ||
| } |
…endently of creation context
chaodhib
force-pushed
the
singleton_static
branch
from
4b9fff7
to
382e85f
Compare
could you please test tools too ? |
|
why not removing these factorys and declare the singletons how we do it everywhere? |
|
@joschiwald Factory pattern is used. It was useful in the past (for VMAP). So I guess it won't hurt to keep it just in case it could be useful again in the future. @jackpoz Done. Starts correctly too. |
|
I don't think it is useful, it is just a file with one singleton and it is
inconsistent to our other singletons
|
|
I don't see any benefit in this change - it actually has negative consequences (exceptionally rare). You see, current approach assumes that you can have different IVMapManager interface implementations (the last time we gained new one was with vmaps overhaul few years ago). If you are going to remove it then also remove the interface, drop 2 from VMapManager2 class name etc... |
Are you talking about the use of the factory pattern or this PR? This PR's gain is just to clean up the code as we go. In the long run, this approach helps to keep the code concise & clean wherever possible. And ok, sure. I will remove the factories, the interface and rename VMapManager2 to VMapManager then. |
|
And then you step right into the other point I made - cluttering history with a change that brings absolutely nothing |
|
I'm not going to argue the benefits of code refactoring with you again, Shauren. Because yes, I remember arguing with you about the same thing in the past. I already spend enough time harassing people about code quality at my day job . I'm not going to do it when I code for fun. I've done the changes. The fact is: It's 200 less LOC and 5 unneeded files removed. Builds, runs and shutdown correctly (server & tools). PS: Since you know C++ better than me, what would had been more constructive is access whether or not the "negative consequences" that you mentioned (= change of the order of the destructor calls which may or may not cause issues) are real. |
|
You should damn well know what my opinion on refactoring only PRs is. Just NO. |
|
none thinks about my CLI branch :( btw https://travis-ci.org/TrinityCore/TrinityCore/builds/202077771#L867 |
|
@jackpoz your CLI branch? |
| { | ||
| static VMapManager gVMapManager; | ||
| return &gVMapManager; | ||
| } |
There was a problem hiding this comment.
VMapManager& VMapManager::Instance()
{
static VMapManager instance;
return instance;
}for more consistence
There was a problem hiding this comment.
createOrGetXMapManager() returns a pointer. not a reference. To change that would require to modify the code of every calls to this method. I would like to avoid that.
Unless you are talking about renaming the method?
|
I'm dropping this. This is minor change and since there is opposition, I don't want to spend more time on this. |
Changes proposed:
Target branch(es): 3.3.5/master
Issues addressed: Does not solve any issue.
Tests performed: Build. Server starts correctly.