Skip to content

Commit f9e78db

Browse files
Vipul-CariappaVipul-Cariappa
committed
Revert "Add Cpp::TakeInterpreter"
This reverts commit da06567.
1 parent d606c8f commit f9e78db

File tree

4 files changed

+37
-84
lines changed

4 files changed

+37
-84
lines changed

‎include/CppInterOp/CppInterOp.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -702,11 +702,6 @@ CreateInterpreter(const std::vector<const char*>& Args = {},
702702
///\returns false on failure or if \c I is not tracked in the stack.
703703
CPPINTEROP_API bool DeleteInterpreter(TInterp_t I = nullptr);
704704

705-
/// Take ownership of an interpreter instance.
706-
///\param[in] I - the interpreter to be taken, if nullptr, returns the last.
707-
///\returns nullptr on failure or if \c I is not tracked in the stack.
708-
CPPINTEROP_API TInterp_t TakeInterpreter(TInterp_t I = nullptr);
709-
710705
/// Activates an instance of an interpreter to handle subsequent API requests
711706
///\param[in] I - the interpreter to be activated.
712707
///\returns false on failure.

‎lib/CppInterOp/CppInterOp.cpp‎

Lines changed: 25 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@
5454
#include "llvm/Support/CommandLine.h"
5555
#include "llvm/Support/Debug.h"
5656
#include "llvm/Support/Error.h"
57-
#include "llvm/Support/ErrorHandling.h"
5857
#include "llvm/Support/ManagedStatic.h"
5958
#include "llvm/Support/Path.h"
6059
#include "llvm/Support/raw_ostream.h"
@@ -139,16 +138,16 @@ struct InterpreterInfo {
139138
InterpreterInfo& operator=(const InterpreterInfo&) = delete;
140139
};
141140

142-
// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables)
141+
// NOLINTBEGIN
143142
// std::deque avoids relocations and calling the dtor of InterpreterInfo.
144-
static llvm::ManagedStatic<std::deque<std::unique_ptr<InterpreterInfo>>>
143+
static llvm::ManagedStatic<std::deque<std::shared_ptr<InterpreterInfo>>>
145144
sInterpreters;
146145
static llvm::ManagedStatic<
147-
std::unordered_map<clang::ASTContext*, InterpreterInfo*>>
146+
std::unordered_map<clang::ASTContext*, std::weak_ptr<InterpreterInfo>>>
148147
sInterpreterASTMap;
149148
static std::recursive_mutex InterpreterStackLock;
150149
static std::recursive_mutex LLVMLock;
151-
// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables)
150+
// NOLINTEND
152151

153152
static InterpreterInfo& getInterpInfo() {
154153
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
@@ -162,20 +161,17 @@ static InterpreterInfo& getInterpInfo(const clang::Decl* D) {
162161
return getInterpInfo();
163162
if (sInterpreters->size() == 1)
164163
return *sInterpreters->back();
165-
return *(*sInterpreterASTMap)[&D->getASTContext()];
164+
return *(*sInterpreterASTMap)[&D->getASTContext()].lock();
166165
}
167166
static InterpreterInfo& getInterpInfo(const void* D) {
168167
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
169-
QualType QT = QualType::getFromOpaquePtr(D);
170-
if (auto* D = QT->getAsTagDecl())
171-
return getInterpInfo(D);
172168
if (!D)
173169
return getInterpInfo();
174170
if (sInterpreters->size() == 1)
175171
return *sInterpreters->back();
176172
for (auto& item : *sInterpreterASTMap) {
177173
if (item.first->getAllocator().identifyObject(D))
178-
return *item.second;
174+
return *item.second.lock();
179175
}
180176
llvm_unreachable(
181177
"This pointer does not belong to any interpreter instance.\n");
@@ -193,7 +189,7 @@ static compat::Interpreter& getInterp(const clang::Decl* D) {
193189
return getInterp();
194190
if (sInterpreters->size() == 1)
195191
return *sInterpreters->back()->Interpreter;
196-
return *(*sInterpreterASTMap)[&D->getASTContext()]->Interpreter;
192+
return *(*sInterpreterASTMap)[&D->getASTContext()].lock()->Interpreter;
197193
}
198194
static compat::Interpreter& getInterp(const void* D) {
199195
return *getInterpInfo(D).Interpreter;
@@ -3342,8 +3338,6 @@ static std::string MakeResourcesPath() {
33423338
TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
33433339
const std::vector<const char*>& GpuArgs /*={}*/) {
33443340
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
3345-
assert(sInterpreters->size() == sInterpreterASTMap->size());
3346-
33473341
std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr);
33483342
std::string ResourceDir = MakeResourcesPath();
33493343
std::vector<const char*> ClingArgv = {"-resource-dir", ResourceDir.c_str(),
@@ -3420,73 +3414,42 @@ TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
34203414
)");
34213415

34223416
sInterpreters->emplace_back(
3423-
std::make_unique<InterpreterInfo>(I, /*Owned=*/true));
3417+
std::make_shared<InterpreterInfo>(I, /*Owned=*/true));
34243418
sInterpreterASTMap->insert(
34253419
{&sInterpreters->back()->Interpreter->getSema().getASTContext(),
3426-
sInterpreters->back().get()});
3420+
sInterpreters->back()});
34273421

3428-
assert(sInterpreters->size() == sInterpreterASTMap->size());
34293422
return I;
34303423
}
34313424

3432-
static inline auto find_interpreter_in_stack(TInterp_t I) {
3433-
return std::find_if(
3434-
sInterpreters->begin(), sInterpreters->end(),
3435-
[&I](const auto& Info) { return Info->Interpreter == I; });
3436-
}
3437-
3438-
static inline auto find_interpreter_in_map(InterpreterInfo* I) {
3439-
return std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(),
3440-
[&](const auto& Item) { return Item.second == I; });
3441-
}
3442-
34433425
bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
34443426
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
3445-
assert(sInterpreters->size() == sInterpreterASTMap->size());
34463427

34473428
if (!I) {
3448-
auto foundAST = find_interpreter_in_map(sInterpreters->back().get());
3449-
assert(foundAST != sInterpreterASTMap->end());
3429+
auto foundAST =
3430+
std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(),
3431+
[](const auto& Item) {
3432+
return Item.second.lock() == sInterpreters->back();
3433+
});
34503434
sInterpreterASTMap->erase(foundAST);
34513435
sInterpreters->pop_back();
34523436
return true;
34533437
}
34543438

3455-
auto found = find_interpreter_in_stack(I);
3439+
auto found =
3440+
std::find_if(sInterpreters->begin(), sInterpreters->end(),
3441+
[&I](const auto& Info) { return Info->Interpreter == I; });
34563442
if (found == sInterpreters->end())
34573443
return false; // failure
34583444

3459-
auto foundAST = find_interpreter_in_map((*found).get());
3460-
assert(foundAST != sInterpreterASTMap->end());
3445+
auto foundAST = std::find_if(
3446+
sInterpreterASTMap->begin(), sInterpreterASTMap->end(),
3447+
[&found](const auto& Item) { return Item.second.lock() == *found; });
34613448
sInterpreterASTMap->erase(foundAST);
34623449
sInterpreters->erase(found);
34633450
return true;
34643451
}
34653452

3466-
TInterp_t TakeInterpreter(TInterp_t I /*=nullptr*/) {
3467-
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
3468-
assert(sInterpreters->size() == sInterpreterASTMap->size());
3469-
3470-
if (!I) {
3471-
auto foundAST = find_interpreter_in_map(sInterpreters->back().get());
3472-
sInterpreterASTMap->erase(foundAST);
3473-
InterpreterInfo* res = sInterpreters->back().release();
3474-
sInterpreters->pop_back();
3475-
return res->Interpreter;
3476-
}
3477-
3478-
auto found = find_interpreter_in_stack(I);
3479-
if (found == sInterpreters->end())
3480-
return nullptr; // failure
3481-
3482-
auto foundAST = find_interpreter_in_map((*found).get());
3483-
sInterpreterASTMap->erase(foundAST);
3484-
InterpreterInfo* res = (*found).release();
3485-
sInterpreters->erase(found);
3486-
assert(sInterpreters->size() == sInterpreterASTMap->size());
3487-
return res->Interpreter;
3488-
}
3489-
34903453
bool ActivateInterpreter(TInterp_t I) {
34913454
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
34923455

@@ -3514,13 +3477,10 @@ TInterp_t GetInterpreter() {
35143477

35153478
void UseExternalInterpreter(TInterp_t I) {
35163479
std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
3480+
assert(sInterpreters->empty() && "sInterpreter already in use!");
35173481
sInterpreters->emplace_back(
3518-
std::make_unique<InterpreterInfo>(static_cast<compat::Interpreter*>(I),
3482+
std::make_shared<InterpreterInfo>(static_cast<compat::Interpreter*>(I),
35193483
/*isOwned=*/false));
3520-
sInterpreterASTMap->insert(
3521-
{&sInterpreters->back()->Interpreter->getSema().getASTContext(),
3522-
sInterpreters->back().get()});
3523-
assert(sInterpreters->size() == sInterpreterASTMap->size());
35243484
}
35253485

35263486
void AddSearchPath(const char* dir, bool isUser, bool prepend) {
@@ -3774,9 +3734,9 @@ std::string ObjToString(const char* type, void* obj) {
37743734
return getInterp(NULLPTR).toString(type, obj);
37753735
}
37763736

3777-
static Decl* InstantiateTemplate(TemplateDecl* TemplateD,
3778-
TemplateArgumentListInfo& TLI, Sema& S,
3779-
bool instantiate_body) {
3737+
Decl* InstantiateTemplate(TemplateDecl* TemplateD,
3738+
TemplateArgumentListInfo& TLI, Sema& S,
3739+
bool instantiate_body) {
37803740
LOCK(getInterpInfo());
37813741
// This is not right but we don't have a lot of options to choose from as a
37823742
// template instantiation requires a valid source location.

‎lib/CppInterOp/CppInterOpInterpreter.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,12 @@ namespace Cpp {
140140
/// CppInterOp Interpreter
141141
///
142142
class Interpreter {
143+
private:
143144
std::unique_ptr<clang::Interpreter> inner;
144145

145-
public:
146146
Interpreter(std::unique_ptr<clang::Interpreter> CI) : inner(std::move(CI)) {}
147147

148+
public:
148149
static std::unique_ptr<Interpreter>
149150
create(int argc, const char* const* argv, const char* llvmdir = nullptr,
150151
const std::vector<std::shared_ptr<clang::ModuleFileExtension>>&

‎unittests/CppInterOp/InterpreterTest.cpp‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
#include "gtest/gtest.h"
2525

2626
#include <algorithm>
27-
#include <utility>
2827

2928
using ::testing::StartsWith;
3029

@@ -151,8 +150,6 @@ TEST(InterpreterTest, Process) {
151150
EXPECT_EQ(Res, CXError_Success);
152151
clang_Value_dispose(CXV);
153152
clang_Interpreter_dispose(CXI);
154-
auto* OldI = Cpp::TakeInterpreter();
155-
EXPECT_EQ(OldI, I);
156153
}
157154

158155
TEST(InterpreterTest, EmscriptenExceptionHandling) {
@@ -332,9 +329,7 @@ if (llvm::sys::RunningOnValgrind())
332329
// Create the interpreter instance.
333330
std::unique_ptr<clang::Interpreter> I =
334331
ExitOnErr(clang::Interpreter::create(std::move(CI)));
335-
336-
auto CPPI = Cpp::Interpreter(std::move(I));
337-
auto* ExtInterp = &CPPI;
332+
auto ExtInterp = I.get();
338333
#endif // CPPINTEROP_USE_REPL
339334

340335
#ifdef CPPINTEROP_USE_CLING
@@ -351,9 +346,12 @@ if (llvm::sys::RunningOnValgrind())
351346

352347
EXPECT_NE(ExtInterp, nullptr);
353348

354-
Cpp::UseExternalInterpreter(ExtInterp);
355-
EXPECT_EQ(ExtInterp, Cpp::GetInterpreter());
356-
EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp));
349+
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
350+
#ifndef _WIN32 // Windows seems to fail to die...
351+
EXPECT_DEATH(Cpp::UseExternalInterpreter(ExtInterp), "sInterpreter already in use!");
352+
#endif // _WIN32
353+
#endif
354+
EXPECT_TRUE(Cpp::GetInterpreter()) << "External Interpreter not set";
357355

358356
#ifndef CPPINTEROP_USE_CLING
359357
I.release();
@@ -368,15 +366,14 @@ TEST(InterpreterTest, MultipleInterpreter) {
368366
#if CLANG_VERSION_MAJOR < 20 && defined(EMSCRIPTEN)
369367
GTEST_SKIP() << "Test fails for Emscipten LLVM 20 builds";
370368
#endif
371-
auto* I = Cpp::CreateInterpreter();
372-
EXPECT_TRUE(I);
369+
370+
EXPECT_TRUE(Cpp::CreateInterpreter());
373371
Cpp::Declare(R"(
374372
void f() {}
375373
)");
376374
Cpp::TCppScope_t f = Cpp::GetNamed("f");
377375

378-
auto* I2 = Cpp::CreateInterpreter();
379-
EXPECT_TRUE(I2);
376+
EXPECT_TRUE(Cpp::CreateInterpreter());
380377
Cpp::Declare(R"(
381378
void ff() {}
382379
)");

0 commit comments

Comments
 (0)