-
Notifications
You must be signed in to change notification settings - Fork 31
Stack the created interpreters. #607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,14 +46,18 @@ | |
#include "llvm/ADT/StringRef.h" | ||
#include "llvm/Demangle/Demangle.h" | ||
#include "llvm/Support/Casting.h" | ||
#include "llvm/Support/CommandLine.h" | ||
#include "llvm/Support/Debug.h" | ||
#include "llvm/Support/raw_os_ostream.h" | ||
|
||
#include <algorithm> | ||
#include <cassert> | ||
#include <iterator> | ||
#include <map> | ||
#include <memory> | ||
#include <set> | ||
#include <sstream> | ||
#include <stack> | ||
#include <string> | ||
|
||
// Stream redirect. | ||
|
@@ -71,30 +75,29 @@ | |
#include <unistd.h> | ||
#endif // WIN32 | ||
|
||
#include <stack> | ||
|
||
namespace Cpp { | ||
|
||
using namespace clang; | ||
using namespace llvm; | ||
using namespace std; | ||
|
||
// Flag to indicate ownership when an external interpreter instance is used. | ||
static bool OwningSInterpreter = true; | ||
static compat::Interpreter* sInterpreter = nullptr; | ||
// Valgrind complains about __cxa_pure_virtual called when deleting | ||
// llvm::SectionMemoryManager::~SectionMemoryManager as part of the dtor chain | ||
// of the Interpreter. | ||
// This might fix the issue https://reviews.llvm.org/D107087 | ||
// FIXME: For now we just leak the Interpreter. | ||
struct InterpDeleter { | ||
~InterpDeleter() = default; | ||
} Deleter; | ||
struct InterpreterInfo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: class 'InterpreterInfo' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] struct InterpreterInfo {
^ |
||
compat::Interpreter* Interpreter = nullptr; | ||
bool isOwned = true; | ||
|
||
// Valgrind complains about __cxa_pure_virtual called when deleting | ||
// llvm::SectionMemoryManager::~SectionMemoryManager as part of the dtor | ||
// chain of the Interpreter. | ||
// This might fix the issue https://reviews.llvm.org/D107087 | ||
// FIXME: For now we just leak the Interpreter. | ||
~InterpreterInfo() = default; | ||
}; | ||
static llvm::SmallVector<InterpreterInfo, 8> sInterpreters; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: variable 'sInterpreters' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables] static llvm::SmallVector<InterpreterInfo, 8> sInterpreters;
^ |
||
|
||
static compat::Interpreter& getInterp() { | ||
assert(sInterpreter && | ||
assert(!sInterpreters.empty() && | ||
vgvassilev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Interpreter instance must be set before calling this!"); | ||
return *sInterpreter; | ||
return *sInterpreters.back().Interpreter; | ||
} | ||
static clang::Sema& getSema() { return getInterp().getCI()->getSema(); } | ||
static clang::ASTContext& getASTContext() { return getSema().getASTContext(); } | ||
|
@@ -2920,19 +2923,54 @@ | |
Args[NumArgs + 1] = nullptr; | ||
llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get()); | ||
} | ||
// FIXME: Enable this assert once we figure out how to fix the multiple | ||
// calls to CreateInterpreter. | ||
// assert(!sInterpreter && "Interpreter already set."); | ||
sInterpreter = I; | ||
|
||
sInterpreters.push_back({I, /*isOwned=*/true}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays] auto Args = std::make_unique<const char*[]>(NumArgs + 2);
^
vgvassilev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return I; | ||
} | ||
|
||
TInterp_t GetInterpreter() { return sInterpreter; } | ||
bool DeleteInterpreter(TInterp_t I /*=nullptr*/) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get());
^
vgvassilev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!I) { | ||
sInterpreters.pop_back(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions] llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get());
^ |
||
return true; | ||
} | ||
|
||
auto* found = | ||
std::find_if(sInterpreters.begin(), sInterpreters.end(), | ||
[&I](const auto& Info) { return Info.Interpreter == I; }); | ||
if (found == sInterpreters.end()) | ||
return false; // failure | ||
|
||
sInterpreters.erase(found); | ||
return true; | ||
} | ||
|
||
bool ActivateInterpreter(TInterp_t I) { | ||
vgvassilev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!I) | ||
return false; | ||
|
||
auto* found = | ||
std::find_if(sInterpreters.begin(), sInterpreters.end(), | ||
[&I](const auto& Info) { return Info.Interpreter == I; }); | ||
if (found == sInterpreters.end()) | ||
return false; | ||
|
||
if (std::next(found) != sInterpreters.end()) // if not already last element. | ||
std::rotate(found, found + 1, sInterpreters.end()); | ||
|
||
return true; // success | ||
} | ||
vgvassilev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
TInterp_t GetInterpreter() { | ||
if (sInterpreters.empty()) | ||
return nullptr; | ||
return sInterpreters.back().Interpreter; | ||
} | ||
|
||
void UseExternalInterpreter(TInterp_t I) { | ||
assert(!sInterpreter && "sInterpreter already in use!"); | ||
sInterpreter = static_cast<compat::Interpreter*>(I); | ||
OwningSInterpreter = false; | ||
assert(sInterpreters.empty() && "sInterpreter already in use!"); | ||
sInterpreters.push_back( | ||
{static_cast<compat::Interpreter*>(I), /*isOwned=*/false}); | ||
} | ||
|
||
void AddSearchPath(const char* dir, bool isUser, bool prepend) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,47 @@ TEST(InterpreterTest, Evaluate) { | |
EXPECT_FALSE(HadError) ; | ||
} | ||
|
||
TEST(InterpreterTest, DeleteInterpreter) { | ||
auto* I1 = Cpp::CreateInterpreter(); | ||
auto* I2 = Cpp::CreateInterpreter(); | ||
auto* I3 = Cpp::CreateInterpreter(); | ||
EXPECT_TRUE(I1 && I2 && I3) << "Failed to create interpreters"; | ||
|
||
EXPECT_EQ(I3, Cpp::GetInterpreter()) << "I3 is not active"; | ||
|
||
EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr)); | ||
EXPECT_EQ(I2, Cpp::GetInterpreter()); | ||
|
||
auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast] auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr] auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "std::uintptr_t" is directly included [misc-include-cleaner] auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
^ |
||
EXPECT_FALSE(Cpp::DeleteInterpreter(I4)); | ||
|
||
EXPECT_TRUE(Cpp::DeleteInterpreter(I1)); | ||
EXPECT_EQ(I2, Cpp::GetInterpreter()) << "I2 is not active"; | ||
} | ||
|
||
TEST(InterpreterTest, ActivateInterpreter) { | ||
EXPECT_FALSE(Cpp::ActivateInterpreter(nullptr)); | ||
auto* Cpp14 = Cpp::CreateInterpreter({"-std=c++14"}); | ||
auto* Cpp17 = Cpp::CreateInterpreter({"-std=c++17"}); | ||
auto* Cpp20 = Cpp::CreateInterpreter({"-std=c++20"}); | ||
|
||
EXPECT_TRUE(Cpp14 && Cpp17 && Cpp20); | ||
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 202002L) | ||
<< "Failed to activate C++20"; | ||
|
||
auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast] auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr] auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));
^ |
||
EXPECT_FALSE(Cpp::ActivateInterpreter(UntrackedI)); | ||
|
||
EXPECT_TRUE(Cpp::ActivateInterpreter(Cpp14)); | ||
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201402L); | ||
|
||
Cpp::DeleteInterpreter(Cpp14); | ||
EXPECT_EQ(Cpp::GetInterpreter(), Cpp20); | ||
|
||
EXPECT_TRUE(Cpp::ActivateInterpreter(Cpp17)); | ||
EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201703L); | ||
} | ||
|
||
TEST(InterpreterTest, Process) { | ||
#ifdef _WIN32 | ||
GTEST_SKIP() << "Disabled on Windows. Needs fixing."; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: class 'InterpreterInfo' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]