-
Notifications
You must be signed in to change notification settings - Fork 39
Added undo command for CppInterOp #513
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
cb99e62
bdbf3a5
4e6ba5f
272039f
24e0fe7
ae95de9
4a777dc
ec2f201
0494249
2dc971c
80ceaa4
c4ac735
f9581c9
2ace298
77489d7
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 |
|---|---|---|
|
|
@@ -3684,4 +3684,15 @@ namespace Cpp { | |
| complete_column); | ||
| } | ||
|
|
||
| int Undo(unsigned N) { | ||
| #ifdef CPPINTEROP_USE_CLING | ||
anutosh491 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| auto& I = getInterp(); | ||
| cling::Interpreter::PushTransactionRAII RAII(&I); | ||
| I.unload(N); | ||
|
Contributor
Author
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. @vgvassilev How do I even check if unload function logs any errors, since its a void function? Should I use try-catch block(but it is disabled in the project) or use system level redirection of the output to stream using pipes(this might be an overkill for capturing)? unload function logs
Contributor
Author
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. @vgvassilev Could you please guide me on how to proceed here?
Contributor
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. You should assume that the operation was successful.
Collaborator
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. Hmmm, I think that's what is being done here https://github.com/compiler-research/CppInterOp/pull/513/files#diff-fb574ca9d2633ca33be71dfa5c0d9f437cfa98768f0e828d5eb789bfcfcef688R3624 so nothing to address here correct ? |
||
| return compat::Interpreter::kSuccess; | ||
| #else | ||
| return getInterp().undo(N); | ||
| #endif | ||
| } | ||
|
|
||
| } // end namespace Cpp | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1615,3 +1615,30 @@ TEST(FunctionReflectionTest, Destruct) { | |
| clang_Interpreter_takeInterpreterAsPtr(I); | ||
| clang_Interpreter_dispose(I); | ||
| } | ||
|
|
||
| TEST(FunctionReflectionTest, UndoTest) { | ||
| #ifdef _WIN32 | ||
| GTEST_SKIP() << "Disabled on Windows. Needs fixing."; | ||
| #endif | ||
| #ifdef EMSCRIPTEN | ||
| GTEST_SKIP() << "Test fails for Emscipten builds"; | ||
| #else | ||
| Cpp::CreateInterpreter(); | ||
| EXPECT_EQ(Cpp::Process("int a = 5;"), 0); | ||
| EXPECT_EQ(Cpp::Process("int b = 10;"), 0); | ||
| EXPECT_EQ(Cpp::Process("int x = 5;"), 0); | ||
| Cpp::Undo(); | ||
|
Collaborator
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. I believe unloading this first transaction here is problematic for destructors, and is unsupported by Cling, which can be seen in the error thrown in the logs. We should drop this call to
Collaborator
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. @kr-2003 can you address this review comment?
Contributor
Author
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. i added two more instructions before calling the first Cpp::Undo(). |
||
| EXPECT_NE(Cpp::Process("int y = x;"), 0); | ||
| EXPECT_EQ(Cpp::Process("int x = 10;"), 0); | ||
| EXPECT_EQ(Cpp::Process("int y = 10;"), 0); | ||
| Cpp::Undo(2); | ||
| EXPECT_EQ(Cpp::Process("int x = 20;"), 0); | ||
| EXPECT_EQ(Cpp::Process("int y = 20;"), 0); | ||
| int ret = Cpp::Undo(100); | ||
| #ifdef CPPINTEROP_USE_CLING | ||
| EXPECT_EQ(ret, 0); | ||
| #else | ||
| EXPECT_EQ(ret, 1); | ||
| #endif | ||
| #endif | ||
| } | ||
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.
If using clang_repl you can return CXError_Failure if it fails. With cling and the c api currently as it stands you cannot. Can you fix this @kr-2003