Fix FIXME: thread-safe initialization of DispatchInitializer#844
Fix FIXME: thread-safe initialization of DispatchInitializer#844voyager-jhk wants to merge 4 commits intocompiler-research:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #844 +/- ##
=======================================
Coverage 79.56% 79.56%
=======================================
Files 11 11
Lines 4013 4013
=======================================
Hits 3193 3193
Misses 820 820 see 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
unittests/CppInterOp/Utils.cpp
Outdated
| // FIXME: Make this threadsafe by moving it as a function static. | ||
| DispatchInitializer g_dispatch_init; | ||
| // Thread-safe initialization using function-local static | ||
| static DispatchInitializer& GetDispatchInitializer() { |
There was a problem hiding this comment.
warning: 'GetDispatchInitializer' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
| static DispatchInitializer& GetDispatchInitializer() { | |
| DispatchInitializer& GetDispatchInitializer() { |
unittests/CppInterOp/Utils.cpp
Outdated
| static DispatchInitializer instance; | ||
| return instance; | ||
| } | ||
| static DispatchInitializer& g_dispatch_init = GetDispatchInitializer(); |
There was a problem hiding this comment.
warning: 'g_dispatch_init' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
| static DispatchInitializer& g_dispatch_init = GetDispatchInitializer(); | |
| DispatchInitializer& g_dispatch_init = GetDispatchInitializer(); |
unittests/CppInterOp/Utils.cpp
Outdated
| static DispatchInitializer instance; | ||
| return instance; | ||
| } | ||
| static DispatchInitializer& g_dispatch_init = GetDispatchInitializer(); |
There was a problem hiding this comment.
warning: variable 'g_dispatch_init' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
static DispatchInitializer& g_dispatch_init = GetDispatchInitializer();
^
unittests/CppInterOp/Utils.cpp
Outdated
| static DispatchInitializer instance; | ||
| return instance; | ||
| } | ||
| DispatchInitializer& g_dispatch_init = GetDispatchInitializer(); |
There was a problem hiding this comment.
warning: variable 'g_dispatch_init' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
DispatchInitializer& g_dispatch_init = GetDispatchInitializer();
^
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
Any chance for a test?
Thanks! I've pushed a fix to restore the initialization trigger. It looks like CI didn't run for the latest commit. Could you please rerun it? |
@voyager-jhk The ci doesn't run automatically for those who are contributing for the first time. It needs to be manually run. You will need to rebase as well to fix the Emscripten ci. |
|
This is a one-time initialisation for the CppInterOpDispatchTests binary and not thread-safe for the tests (this is not called by each test). We need a single instance creation in Utils.cpp for the API to be loaded at runtime... Unless this fixes some use of |
|
clang-tidy review says "All clean, LGTM! 👍" |
4 participants
Fix FIXME regarding thread-safe initialization of DispatchInitializer.
This change replaces the global static instance with a function-local
static to guarantee thread-safe initialization according to C++11.
The initializer is now created via GetDispatchInitializer().