-
Notifications
You must be signed in to change notification settings - Fork 34
Add test for verifying exception handling for emscripten builds #543
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
Add test for verifying exception handling for emscripten builds #543
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
=======================================
Coverage 75.94% 75.94%
=======================================
Files 9 9
Lines 3646 3646
=======================================
Hits 2769 2769
Misses 877 877 🚀 New features to boost your workflow:
|
The fail test look completely unrelated The test I added for the emscripten build passes here (https://github.com/compiler-research/CppInterOp/actions/runs/14192033343/job/39758739024?pr=543#step:8:569) |
Hi, I realize some cppyy based tests are failing. Not sure what's going wrong but possibly @Vipul-Cariappa can help me out here ? |
Hi @anutosh491 The cppyy jobs should pass again once compiler-research/cppyy#140 is in. There is some work being done to get some extra cppyy tests passing. |
Sorry about those tests! Should be ready in a couple of minutes and a rerun can be triggered |
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.
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.
LGTM! One minor comment: Would be good to add the test to the end of the file
Hey @vgvassilev,
If you remember, we were discussing the changes needed to handle the LLVMargs from FrontendOpts for clang-repl the other day.
This led to me coming up with this PR on llvm llvm/llvm-project#132670
But I think CppInterOp does take some inspiration from the above in the past and has added a block for handling the LLVM args
CppInterOp/lib/Interpreter/CppInterOp.cpp
Lines 2962 to 2975 in e1ace51
Hence we should be able to process any exception handling case for our emscripten build through cppinterop (though clang-repl in the browser itself won't be able to handle these)
This adds a simple test case for the same. Which we can possibly remove along with the above code block .... once this in completely done in clang-repl itself (possibly like how I tried to achieve it in the above linked PR)