-
Notifications
You must be signed in to change notification settings - Fork 36
Initial merge with xeus-clang-repl (mini) #46
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
1844458
e5ec265
dd29feb
0b77bb6
b59a960
07f3330
15e0363
8880063
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 |
---|---|---|
|
@@ -4,9 +4,9 @@ channels: | |
- https://repo.mamba.pm/conda-forge | ||
dependencies: | ||
- nlohmann_json | ||
- xeus-lite | ||
- xeus-lite <2.0 | ||
- xeus >=3.0.5,<4.0 | ||
- xtl >=0.7,<0.8 | ||
- llvm =16.0.6 | ||
- CppInterOp | ||
- cpp-argparse | ||
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. Should we pin this to <3.1 too ? |
||
- pugixml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
|
||
#include "xcpp/xmime.hpp" | ||
|
||
#include "xeus/xinterpreter.hpp" | ||
|
||
namespace nl = nlohmann; | ||
|
||
namespace xcpp | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
#include <string> | ||
#include <vector> | ||
|
||
#include <clang/Interpreter/Interpreter.h> | ||
#include "clang/Interpreter/CppInterOp.h" // from CppInterOp package | ||
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. Deos CppInterop place a header in the 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. Yeah I was thinking about this too ! 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. Nope, it is not but that's the design of the package - we should change that in the new versions if needed. EDIT: It does not place a header in the 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 am confused. Is the CppInterpop header placed there? 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. Ah, wait, are we having a common place for all headers? If everything is under include then it copies it there… 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 am still confused by why 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. Yes, not a good choice. We will fix this in the next release of CppInterOp. Do we want to block progress because of that? 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.
Thanks! |
||
|
||
#include <nlohmann/json.hpp> | ||
|
||
|
@@ -76,10 +76,6 @@ namespace xcpp | |
void init_preamble(); | ||
void init_magic(); | ||
|
||
std::string get_stdopt(int argc, const char* const* argv); | ||
|
||
std::unique_ptr<clang::Interpreter> m_interpreter; | ||
|
||
std::string m_version; | ||
|
||
xmagics_manager xmagics; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,16 @@ | ||
{ | ||
"display_name": "cpp 14 (xcpp)", | ||
"env": { | ||
"PATH":"@XEUS_CPP_PATH@", | ||
"LD_LIBRARY_PATH":"@XEUS_CPP_LD_LIBRARY_PATH@" | ||
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 merged, but I wonder why we need to set these up exactly, if it wasn't required before? 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. Hey @alexander-penev I actually have a doubt along similar lines. I understand how something like resource dir can contribute and hence it is good to have it introduced in argv as done in the PR ! But I am not sure how |
||
}, | ||
"argv": [ | ||
"@XEUS_CPP_KERNELSPEC_PATH@xcpp", | ||
"-f", | ||
"{connection_file}" | ||
"{connection_file}", | ||
"-resource-dir", "@XEUS_CPP_RESOURCE_DIR@", | ||
"-I", "@CMAKE_INSTALL_PREFIX@/include", | ||
"-std=c++14"@XEUS_CPP_OMP@ | ||
], | ||
"language": "cpp", | ||
"metadata": {"debugger": false | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
What is the benefit of this version compared to the previous one?
(trying to understand better because keeping cmake files of xeus-based kernels similar makes it easier to make changes across all kernels).