Skip to content
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

Export __clang_Interpreter_SetValueNoAlloc to allow automatic printf in xeus-cpp-lite #425

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anutosh491
Copy link
Collaborator

Description

Check

  1. Update cppinterop to support automatic printf in xeus-cpp-lite emscripten-forge/recipes#1515
  2. Remove pinning cxx_version in get_stdopt for xeus-cpp-lite xeus-cpp#202
  3. Remove pinning c++ version from get_stdopt xeus-cpp#203

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

  • I have read the contribution guide recently

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.96%. Comparing base (0dc7d72) to head (53a832a).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #425   +/-   ##
=======================================
  Coverage   70.96%   70.96%           
=======================================
  Files           9        9           
  Lines        3541     3541           
=======================================
  Hits         2513     2513           
  Misses       1028     1028           

@anutosh491
Copy link
Collaborator Author

Actually this can be seen in the two links

  1. Cppinterop: https://compiler-research.org/CppInterOp/lab/index.html

  2. xeus-cpp: https://compiler-research.github.io/xeus-cpp/lab/index.html

Try automatic printf (maybe type 10 and execute the shell )

it would error out for cppinterop but not for xeus-cpp (now an output is not expected cause its not yet implemented https://github.com/llvm/llvm-project/blob/73dd730fb9403ca648a46b489bf04e27b2a93840/clang/lib/Interpreter/Value.cpp#L258-L266 but whenever it would be we are ready to support it and this would be required.)

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test demonstrating this now works?

@anutosh491
Copy link
Collaborator Author

Can we add a test demonstrating this now works?

Well unfortunately this is one thing I am not well versed with :(

Never written any concrete "tests' as such for an emscripten use case and also I don't think any other xeus-lite kernels support testing right now.

I shall raise this point in front of xeus-lite maintainers today and see if there is something we can do !

@vgvassilev
Copy link
Contributor

We could add a unit test that uses this symbol and it will fail as part of the ci, right?

@anutosh491
Copy link
Collaborator Author

Sorry for not being able to get back here. Not sure I know how to test this well enough as of now and also focusing more on the implementation part than testing these days. There is some kind of testing framework (as in cell by cell) in jupyterlite-xeus which is already being put to use for xeus-python and we probably need to introduce the same for xeus-cpp.

That being said although I don't know how to test this through the Ci right away, I know for sure that we need it.

If you open cppinterop's lite deployment, you shall find that we fail to compute the C++ version being supported

image

This won't be seen in xeus-cpp's lite deployment cause the same change is added through a patch on cppinterop's recipe on emscripten-forge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants