-
Notifications
You must be signed in to change notification settings - Fork 26
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
Initial merge with xeus-clang-repl - Capture outputs #59
Initial merge with xeus-clang-repl - Capture outputs #59
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
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.
clang-tidy made some suggestions
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
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.
Not sure to understand why an additional capture is required around Cpp::process
. Isn't the initial cpature of std::cout
and std::cerr
enough?
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
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.
clang-tidy made some suggestions
~StreamRedirectRAII() { | ||
std::string out = Cpp::EndStdStreamCapture(); | ||
*err = Cpp::EndStdStreamCapture(); | ||
std::cout << out; |
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.
We should probably print out where err
is printed...
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.
Let's address that in a separate PR.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com>
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.
clang-tidy made some suggestions
Hey @alexander-penev could you maybe squash the commits into a couple meaningful ones. Would make it easier for reviewing ! |
Often squashing loses the previous comments. Maybe we can squash on merge. If you click on the “files changed” tab you will see everything squashed. |
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.
LGTM, but I still don't understand why this is needed.
Are=> this is for capturing fprintf, thanks @vgvassilev for the offline explanationkStdErr
andkStdOut
different beasts fromstd::cerr
andstd::cout
?- Also, why capturing / restoring them on each execute request instead of doing it upon creation / destruction of the interpreter?
No description provided.