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

Enable the %%file xmagic #181

Merged
merged 8 commits into from
Nov 21, 2024
Merged

Conversation

faze-geek
Copy link
Contributor

@faze-geek faze-geek commented Nov 18, 2024

Description

Towards #176

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@faze-geek
Copy link
Contributor Author

Let me know if I need to add more tests. I wrote them for whatever functionalities were supported in the documentation.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.80%. Comparing base (859a1b9) to head (41b13d8).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   80.78%   80.80%   +0.01%     
==========================================
  Files          19       19              
  Lines         973      974       +1     
  Branches       93       93              
==========================================
+ Hits          786      787       +1     
  Misses        187      187              
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.39% <100.00%> (+0.04%) ⬆️
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.39% <100.00%> (+0.04%) ⬆️
---- 🚨 Try these New Features:

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491
Copy link
Collaborator

Hmmm, I know that EMSCRIPTEN supports file utilities (through both libc and libcxx) making use of the virtual file system it provides. So technically we should be able to compile and run this using std::ofstream

But untill xeus-cpp-lite takes good shape (and we don't have a means to test the above out) I would keep this out of emscripten.

@anutosh491
Copy link
Collaborator

Please make the following changes

  1. In cmakelists.txt : Move oss header & source to the not emscripten case.
set(XEUS_CPP_HEADERS
    ....
    #src/xmagics/os.hpp
)

set(XEUS_CPP_SRC
    ....
    src/xinterpreter.cpp
    src/xmagics/os.cpp
    ....
)

if(NOT EMSCRIPTEN)
    list(APPEND XEUS_CPP_SRC
        src/xmagics/xassist.hpp
        src/xmagics/xassist.cpp
    )
endif()
  1. in src/xinterpreter.cpp : Do the same as above.
#include "xmagics/os.hpp"
#ifndef EMSCRIPTEN
#include "xmagics/xassist.hpp"
#endif

@anutosh491
Copy link
Collaborator

Apart from that. The tests look decent to me. Maybe @mcbarton could also review this for us !

@vgvassilev
Copy link
Contributor

@mcbarton, do we not have code coverage bots checking the test suite coverage for xeus-cpp? I remember we had that but maybe I am confusing myself...

@mcbarton
Copy link
Collaborator

mcbarton commented Nov 18, 2024

@mcbarton, do we not have code coverage bots checking the test suite coverage for xeus-cpp? I remember we had that but maybe I am confusing myself...

@vgvassilev Isn't the test coverage being shown in this message? #181 (comment)

@vgvassilev
Copy link
Contributor

@mcbarton, do we not have code coverage bots checking the test suite coverage for xeus-cpp? I remember we had that but maybe I am confusing myself...

@vgvassilev Isn't the test coverage being shown in this message? #181 (comment)

Sorry, I am blind... In clad we had these listed as part of the bot status...

Copy link
Contributor

@github-actions github-actions bot left a 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

src/xinterpreter.cpp Show resolved Hide resolved
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

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

The tests look decent to me. Approving !

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491
Copy link
Collaborator

I am guessing this is ready ?

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev
Copy link
Contributor

The suggested by clang-format changes look good.

@faze-geek
Copy link
Contributor Author

faze-geek commented Nov 21, 2024

The suggested by clang-format changes look good.

Done !

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev vgvassilev merged commit f98cca3 into compiler-research:main Nov 21, 2024
10 checks passed
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.

5 participants