Fix Thread-Safety #721
Fix Thread-Safety #721
Conversation
Vipul-Cariappa
requested review from
vgvassilev,
aaronj0,
anutosh491 and
mcbarton
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (IsSomeThreadWriting == 0) | ||
| return; | ||
| IsSomeThreadWritingMutex.unlock(); | ||
| std::this_thread::sleep_for(0ms); // give control to other threads |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:41:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <bits/chrono.h>
+ #if CLANG_VERSION_MAJOR >= 19| if (IsSomeThreadWriting == 0) | ||
| return; | ||
| IsSomeThreadWritingMutex.unlock(); | ||
| std::this_thread::sleep_for(0ms); // give control to other threads |
There was a problem hiding this comment.
warning: no header providing "std::this_thread::sleep_for" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:41:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <thread>
+ #if CLANG_VERSION_MAJOR >= 19| // return getSema(D).getASTContext(); | ||
| // } | ||
|
|
||
| int Declare(compat::Interpreter& I, const char* code, bool silent); |
There was a problem hiding this comment.
warning: function 'Declare' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int Declare(compat::Interpreter& I, const char* code, bool silent); | |
| static int Declare(compat::Interpreter& I, const char* code, bool silent); |
There was a problem hiding this comment.
Cannot do this, Ref:
CppInterOp/lib/CppInterOp/CXCppInterOp.cpp
Line 327 in 0aa859b
There was a problem hiding this comment.
Why Declare there is different than for example process:
CppInterOp/lib/CppInterOp/CXCppInterOp.cpp
Line 342 in 0aa859b
lib/CppInterOp/CppInterOp.cpp
Outdated
| } | ||
|
|
||
| static void InstantiateFunctionDefinition(Decl* D) { | ||
| static void InstantiateFunctionDefinition(Decl* D, Interpreter& I) { |
There was a problem hiding this comment.
warning: no header providing "Cpp::Interpreter" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:12:
+ #include "CppInterOpInterpreter.h"| auto* Within = llvm::dyn_cast<clang::DeclContext>(D); | ||
|
|
||
| auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); | ||
| auto* ND = Cpp_utils::Lookup::Named(&I.getSema(), name, Within); |
There was a problem hiding this comment.
warning: no header providing "Cpp::utils::Lookup::Named" is directly included [misc-include-cleaner]
auto* ND = Cpp_utils::Lookup::Named(&I.getSema(), name, Within);
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| } | ||
|
|
||
| TCppIndex_t GetNumBases(TCppScope_t klass) { | ||
| auto* D = (Decl*)klass; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (Decl*)klass;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
|
|
||
| TCppType_t GetTypeFromScope(TCppScope_t klass) { | ||
| if (!klass) | ||
| return 0; |
There was a problem hiding this comment.
warning: use nullptr [modernize-use-nullptr]
| return 0; | |
| return nullptr; |
lib/CppInterOp/CppInterOp.cpp
Outdated
| if (!klass) | ||
| return 0; | ||
|
|
||
| auto* D = (Decl*)klass; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (Decl*)klass;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: 'gWrapperSerial' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; | |
| std::atomic<unsigned long long> gWrapperSerial = 0LL; |
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: no header providing "std::atomic" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:41:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <atomic>
+ #if CLANG_VERSION_MAJOR >= 19
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| static std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: variable 'gWrapperSerial' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static std::atomic<unsigned long long> gWrapperSerial = 0LL;
^There was a problem hiding this comment.
Shouldn't that be per-interpreter state?
There was a problem hiding this comment.
Yes, ideally it should be. But we cannot make it per-interpreter if we want to support cling. At least it is complicated. I say will leave as a static atomic.
| std::lock_guard<std::mutex> Lock(gWrapperStoreMutex); | ||
| auto R = gWrapperStore.find(FD); | ||
| if (R != gWrapperStore.end()) | ||
| return (JitCall::GenericCall)R->second; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::GenericCall)R->second;
^| std::lock_guard<std::mutex> Lock(gDtorWrapperStoreMutex); | ||
| auto I = gDtorWrapperStore.find(D); | ||
| if (I != gDtorWrapperStore.end()) | ||
| return (JitCall::DestructorCall)I->second; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::DestructorCall)I->second;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| if (!Cpp::Declare(instance.c_str(), /*silent=*/false, interp)) { | ||
| auto* VD = static_cast<VarDecl*>(Cpp::GetNamed(id, nullptr, interp)); | ||
| if (!Cpp::Declare(*IF, instance.c_str(), /*silent=*/false)) { | ||
| auto* VD = static_cast<VarDecl*>(Cpp::GetNamed(*IF, id, nullptr)); |
There was a problem hiding this comment.
warning: do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-static-cast-downcast]
| auto* VD = static_cast<VarDecl*>(Cpp::GetNamed(*IF, id, nullptr)); | |
| auto* VD = dynamic_cast<VarDecl*>(Cpp::GetNamed(*IF, id, nullptr)); |
| llvm::InitializeAllTargets(); | ||
| llvm::InitializeAllTargetMCs(); | ||
| llvm::InitializeAllAsmPrinters(); | ||
| static std::once_flag call_once_flag; |
There was a problem hiding this comment.
warning: no header providing "std::once_flag" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOpInterpreter.h:25:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <mutex>
+ #if CLANG_VERSION_MAJOR >= 19| llvm::InitializeAllTargetMCs(); | ||
| llvm::InitializeAllAsmPrinters(); | ||
| static std::once_flag call_once_flag; | ||
| std::call_once(call_once_flag, []() { |
There was a problem hiding this comment.
warning: no header providing "std::call_once" is directly included [misc-include-cleaner]
std::call_once(call_once_flag, []() {
^| const clang::CompilerInstance* getCI() const { return getCompilerInstance(); } | ||
|
|
||
| clang::Sema& getSema() const { return getCI()->getSema(); } | ||
| clang::ASTContext& getASTContext() const { return getSema().getASTContext(); } |
There was a problem hiding this comment.
warning: function 'getASTContext' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::ASTContext& getASTContext() const { return getSema().getASTContext(); } | |
| [[nodiscard]] clang::ASTContext& getASTContext() const { return getSema().getASTContext(); } |
| #include "gtest/gtest.h" | ||
|
|
||
| #include <chrono> | ||
| #include <thread> |
There was a problem hiding this comment.
warning: included header chrono is not used directly [misc-include-cleaner]
| #include <thread> | |
| #include <thread> |
|
|
||
| #include <chrono> | ||
| #include <thread> | ||
|
|
There was a problem hiding this comment.
warning: included header thread is not used directly [misc-include-cleaner]
| using namespace clang; | ||
|
|
||
| TEST(ScopeReflectionTest, IsEnumScope) { | ||
| void ScopeReflectionTest_IsEnumScope() { |
There was a problem hiding this comment.
warning: function 'ScopeReflectionTest_IsEnumScope' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void ScopeReflectionTest_IsEnumScope() { | |
| static void ScopeReflectionTest_IsEnumScope() { |
| #include <atomic> | ||
| #include <cassert> | ||
| #include <chrono> | ||
| #include <cstddef> |
There was a problem hiding this comment.
warning: included header chrono is not used directly [misc-include-cleaner]
| #include <cstddef> | |
| #include <cstddef> |
| if (IsSomeThreadWriting == 0) | ||
| return; | ||
| IsSomeThreadWritingMutex.unlock(); | ||
| std::this_thread::sleep_for(0ms); // give control to other threads |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:42:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <bits/chrono.h>
+ #if CLANG_VERSION_MAJOR >= 19| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
warning: variable 'gWrapperSerial' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::atomic<unsigned long long> gWrapperSerial = 0LL;
^| } | ||
|
|
||
| TEST(ScopeReflectionTest, IsEnumConstant) { | ||
| void ScopeReflectionTest_IsEnumConstant() { |
There was a problem hiding this comment.
warning: function 'ScopeReflectionTest_IsEnumConstant' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void ScopeReflectionTest_IsEnumConstant() { | |
| static void ScopeReflectionTest_IsEnumConstant() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, IsEnumType) { | ||
| void EnumReflectionTest_IsEnumType() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_IsEnumType' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_IsEnumType() { | |
| static void EnumReflectionTest_IsEnumType() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetIntegerTypeFromEnumScope) { | ||
| void EnumReflectionTest_GetIntegerTypeFromEnumScope() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetIntegerTypeFromEnumScope' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetIntegerTypeFromEnumScope() { | |
| static void EnumReflectionTest_GetIntegerTypeFromEnumScope() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetIntegerTypeFromEnumType) { | ||
| void EnumReflectionTest_GetIntegerTypeFromEnumType() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetIntegerTypeFromEnumType' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetIntegerTypeFromEnumType() { | |
| static void EnumReflectionTest_GetIntegerTypeFromEnumType() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetEnumConstants) { | ||
| void EnumReflectionTest_GetEnumConstants() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetEnumConstants' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetEnumConstants() { | |
| static void EnumReflectionTest_GetEnumConstants() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetEnumConstantType) { | ||
| void EnumReflectionTest_GetEnumConstantType() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetEnumConstantType' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetEnumConstantType() { | |
| static void EnumReflectionTest_GetEnumConstantType() { |
| } | ||
|
|
||
| TEST(EnumReflectionTest, GetEnumConstantValue) { | ||
| void EnumReflectionTest_GetEnumConstantValue() { |
There was a problem hiding this comment.
warning: function 'EnumReflectionTest_GetEnumConstantValue' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void EnumReflectionTest_GetEnumConstantValue() { | |
| static void EnumReflectionTest_GetEnumConstantValue() { |
lib/CppInterOp/CppInterOp.cpp
Outdated
| InterpreterInfo(const InterpreterInfo&) = delete; | ||
| InterpreterInfo& operator=(const InterpreterInfo&) = delete; | ||
|
|
||
| clang::Sema& getSema() const { return Interpreter->getSema(); } |
There was a problem hiding this comment.
warning: function 'getSema' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::Sema& getSema() const { return Interpreter->getSema(); } | |
| [[nodiscard]] clang::Sema& getSema() const { return Interpreter->getSema(); } |
lib/CppInterOp/CppInterOp.cpp
Outdated
| InterpreterInfo& operator=(const InterpreterInfo&) = delete; | ||
|
|
||
| clang::Sema& getSema() const { return Interpreter->getSema(); } | ||
| clang::ASTContext& getASTContext() const { |
There was a problem hiding this comment.
warning: function 'getASTContext' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::ASTContext& getASTContext() const { | |
| [[nodiscard]] clang::ASTContext& getASTContext() const { |
| auto* Within = llvm::dyn_cast<clang::DeclContext>(D); | ||
|
|
||
| auto* ND = Cpp_utils::Lookup::Named(&getSema(D), name, Within); | ||
| auto* ND = Cpp_utils::Lookup::Named(&I.getSema(), name, Within); |
There was a problem hiding this comment.
warning: no header providing "Cpp::utils::Lookup::Named" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:12:
+ #include "CppInterOpInterpreter.h"
lib/CppInterOp/CppInterOp.cpp
Outdated
| // encompassed in an anonymous namespace as follows. | ||
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| std::atomic<unsigned long long> gWrapperSerial = |
There was a problem hiding this comment.
warning: variable 'gWrapperSerial' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::atomic<unsigned long long> gWrapperSerial =
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| std::lock_guard<std::mutex> Lock(gWrapperStoreMutex); | ||
| auto R = gWrapperStore.find(FD); | ||
| if (R != gWrapperStore.end()) | ||
| return (JitCall::GenericCall) |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::GenericCall)
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| std::lock_guard<std::mutex> Lock(gDtorWrapperStoreMutex); | ||
| auto I = gDtorWrapperStore.find(D); | ||
| if (I != gDtorWrapperStore.end()) | ||
| return (JitCall::DestructorCall) |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::DestructorCall)
^| } | ||
|
|
||
| TEST(EnumReflectionTest, EnumReflectionTest) { | ||
| std::vector<std::pair<const char*, void (*)()>> fns = { |
There was a problem hiding this comment.
warning: no header providing "std::pair" is directly included [misc-include-cleaner]
unittests/CppInterOp/EnumReflectionTest.cpp:7:
+ #include <utility>| using namespace clang; | ||
|
|
||
| TEST(FunctionReflectionTest, GetClassMethods) { | ||
| static Cpp::TInterp_t I = nullptr; |
There was a problem hiding this comment.
warning: variable 'I' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static Cpp::TInterp_t I = nullptr;
^| )"; | ||
|
|
||
| GetAllTopLevelDecls(code, Decls); | ||
| Cpp::TInterp_t I1 = GetAllTopLevelDecls(code, Decls); |
There was a problem hiding this comment.
warning: Value stored to 'I1' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
Cpp::TInterp_t I1 = GetAllTopLevelDecls(code, Decls);
^Additional context
unittests/CppInterOp/FunctionReflectionTest.cpp:53: Value stored to 'I1' during its initialization is never read
Cpp::TInterp_t I1 = GetAllTopLevelDecls(code, Decls);
^| EXPECT_FALSE(Cpp::Process(code.c_str(), I)); | ||
| const char* str = "std::make_unique<int,int>"; | ||
| auto* Instance1 = (Decl*)Cpp::InstantiateTemplateFunctionFromString(str); | ||
| auto* Instance1 = (Decl*)Cpp::InstantiateTemplateFunctionFromString(str, I); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* Instance1 = (Decl*)Cpp::InstantiateTemplateFunctionFromString(str, I);
^
| I = Cpp::CreateInterpreter({"-include", "new"}); | ||
| EXPECT_TRUE(I); | ||
|
|
||
| std::vector<std::pair<const char*, void (*)()>> fns = { |
There was a problem hiding this comment.
warning: no header providing "std::pair" is directly included [misc-include-cleaner]
unittests/CppInterOp/FunctionReflectionTest.cpp:16:
+ #include <utility>|
|
||
| TEST(ScopeReflectionTest, Demangle) { | ||
| // NOLINENEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) | ||
| static Cpp::TInterp_t I = nullptr; |
There was a problem hiding this comment.
warning: variable 'I' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static Cpp::TInterp_t I = nullptr;
^|
|
||
| TEST(TypeReflectionTest, GetTypeAsString) { | ||
| // NOLINENEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) | ||
| static Cpp::TInterp_t I = nullptr; |
There was a problem hiding this comment.
warning: variable 'I' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static Cpp::TInterp_t I = nullptr;
^|
|
||
| #include <algorithm> | ||
| #include <chrono> | ||
| #include <future> |
There was a problem hiding this comment.
warning: included header chrono is not used directly [misc-include-cleaner]
| #include <future> | |
| #include <future> |
| const std::vector<const char*>& interpreter_args /* = {} */) { | ||
| Cpp::CreateInterpreter(interpreter_args); | ||
| Cpp::TInterp_t I = Cpp::CreateInterpreter(interpreter_args); | ||
| auto* Interp = static_cast<compat::Interpreter*>(I); |
There was a problem hiding this comment.
warning: no header providing "compat::Interpreter" is directly included [misc-include-cleaner]
unittests/CppInterOp/Utils.cpp:2:
- #include "CppInterOp/CppInterOp.h"
+ #include "/github/workspace/lib/CppInterOp/Compatibility.h"
+ #include "CppInterOp/CppInterOp.h"| std::vector<std::pair<const char*, void (*)()>>& fns, unsigned runners) { | ||
| std::vector<std::future<void>> futures; | ||
| std::vector<const char*> names; | ||
| for (size_t i = 0, size = fns.size(); i < size;) { |
There was a problem hiding this comment.
warning: no header providing "size_t" is directly included [misc-include-cleaner]
unittests/CppInterOp/Utils.cpp:13:
- #include <future>
+ #include <cstddef>
+ #include <future>| i++; | ||
| continue; | ||
| } | ||
| assert(futures.size() == names.size()); |
There was a problem hiding this comment.
warning: no header providing "assert" is directly included [misc-include-cleaner]
unittests/CppInterOp/Utils.cpp:12:
- #include <chrono>
+ #include <cassert>
+ #include <chrono>| } | ||
| assert(futures.size() == names.size()); | ||
| for (size_t i = 0, size = futures.size(); i < size; i++) { | ||
| if (futures[0].wait_for(0ms) == std::future_status::ready) { |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
unittests/CppInterOp/Utils.cpp:12:
- #include <chrono>
+ #include <bits/chrono.h>
+ #include <chrono>|
|
||
| TEST(VariableReflectionTest, GetDatamembers) { | ||
| // NOLINENEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) | ||
| static Cpp::TInterp_t I = nullptr; |
There was a problem hiding this comment.
warning: variable 'I' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static Cpp::TInterp_t I = nullptr;
^|
|
||
| TEST(VariableReflectionTest, DatamembersWithAnonymousStructOrUnion) { | ||
| void | ||
| VariableReflectionTest_DatamembersWithAnonymousStructOrUnion() { |
There was a problem hiding this comment.
warning: function 'VariableReflectionTest_DatamembersWithAnonymousStructOrUnion' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
unittests/CppInterOp/VariableReflectionTest.cpp:119:
- void
+ static void
Vipul-Cariappa
force-pushed
the
wip/Thread
branch
2 times, most recently
from
54695d4 to
501440b
Compare
| #include <algorithm> | ||
| #include <cassert> | ||
| #include <chrono> | ||
| #include <cstddef> |
There was a problem hiding this comment.
warning: included header chrono is not used directly [misc-include-cleaner]
| #include <cstddef> | |
| #include <cstddef> |
| } | ||
| assert(futures.size() == names.size()); | ||
| for (size_t i = 0, size = futures.size(); i < size; i++) { | ||
| if (futures[0].wait_for(0ms) == std::future_status::ready) { |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
unittests/CppInterOp/Utils.cpp:12:
- #include <cassert>
+ #include <bits/chrono.h>
+ #include <cassert>|
|
||
| TEST(VariableReflectionTest, GetVariableOffset) { | ||
| void | ||
| VariableReflectionTest_GetVariableOffset() { |
There was a problem hiding this comment.
warning: function 'VariableReflectionTest_GetVariableOffset' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
unittests/CppInterOp/VariableReflectionTest.cpp:273:
- void
+ static void|
|
||
| TEST(VariableReflectionTest, VariableOffsetsWithInheritance) { | ||
| void | ||
| VariableReflectionTest_VariableOffsetsWithInheritance() { |
There was a problem hiding this comment.
warning: function 'VariableReflectionTest_VariableOffsetsWithInheritance' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
unittests/CppInterOp/VariableReflectionTest.cpp:390:
- void
+ static void
Vipul-Cariappa
force-pushed
the
wip/Thread
branch
2 times, most recently
from
3d262b2 to
1fb2714
Compare
Having static locals can be risky and we might want to move them into the InterpreterInfo class
We now globally lock all the interpreters when we iterate over all the interpreters' bump allocators to resolve which interpreter a `QualType` belongs to. Similarly, we also lock all the interpreters to resolve the interpreter from a `Decl`. I have also adopted most of the tests to run in parallel, hopefully detecting failures early.
Vipul-Cariappa
force-pushed
the
wip/Thread
branch
from
1fb2714 to
ec0fd22
Compare
| } | ||
| assert(futures.size() == names.size()); | ||
| for (size_t i = 0, size = futures.size(); i < size; i++) { | ||
| if (futures[0].wait_for(0ms) == std::future_status::ready) { |
There was a problem hiding this comment.
warning: no header providing "std::operator""ms" is directly included [misc-include-cleaner]
unittests/CppInterOp/Utils.cpp:14:
- #include <cassert>
+ #include <bits/chrono.h>
+ #include <cassert>
| sInterpreterASTMap; | ||
| static std::recursive_mutex InterpreterStackLock; | ||
| static std::recursive_mutex LLVMLock; | ||
| static int IsSomeThreadWriting = 0; |
There was a problem hiding this comment.
We need probably a better name here IsThreadWriting is not too bad.
| static int IsSomeThreadWriting = 0; | ||
| static std::mutex IsSomeThreadWritingMutex; | ||
| static std::mutex InterpreterStackLock; | ||
| static std::mutex LLVMLock; |
There was a problem hiding this comment.
Please add a comment explaining why these variables is fine to be process-wide globals.
| static std::mutex LLVMLock; | ||
| // NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) | ||
|
|
||
| class LOCK { |
| LOCK operator=(LOCK&&) = delete; | ||
| }; | ||
|
|
||
| struct WAIT_ON_ALL { |
There was a problem hiding this comment.
All caps again, we need doxygen style documentation.
| // getInterpInfo(result); // getASTContext may read wrong data | ||
| // if definition is being written, | ||
| // and the declaration has already been written | ||
| WAIT_ON_ALL WAIT; |
There was a problem hiding this comment.
We should name that as somethingRaii.
| auto& IF = getInterpInfo(D); | ||
| auto& C = IF.getASTContext(); |
| TCppIndex_t GetNumBases(TCppScope_t klass) { | ||
| auto* D = (Decl*)klass; | ||
|
|
||
| static TCppIndex_t GetNumBases(compat::Interpreter& I, |
There was a problem hiding this comment.
Why we have these clones of their counterparts taking I. I mean I know why but why only a handful of them and not all?
| namespace { | ||
| static unsigned long long gWrapperSerial = 0LL; | ||
| // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) | ||
| std::atomic<unsigned long long> gWrapperSerial = 0LL; |
There was a problem hiding this comment.
Should that not be unsigned long long?
| using namespace clang; | ||
|
|
||
| TEST(ScopeReflectionTest, IsEnumScope) { | ||
| static void EnumReflectionTest_IsEnumScope() { |
There was a problem hiding this comment.
These changes are incorrect. We should ignore clang-tidy here.
| Cpp::TCppScope_t globalscope = Cpp::GetScope("", nullptr, I); | ||
| Cpp::TCppScope_t Animals_scope = Cpp::GetScope("Animals", nullptr, I); | ||
| Cpp::TCppScope_t myClass_scope = Cpp::GetScope("myClass", nullptr, I); | ||
| Cpp::TCppScope_t unsupported_scope = Cpp::GetScope("myVariable", nullptr, I); |
There was a problem hiding this comment.
I'd think the interfaces became very clunky here. We need something better...
This PR fixes the problems with the old implementation.
We now globally lock all the interpreters when we iterate over all the interpreters' bump allocators to resolve which interpreter a
QualTypebelongs to. Similarly, we also lock all the interpreters to resolve the interpreter from aDecl.I have also adopted most of the tests to run in parallel, hopefully detecting failures early.
Notes to myself:
InterpreterTest.MultipleInterpretersfor LLVM 18. I have seen a crash in CI.