Add error propagation to interpreter creation logic #553

anutosh491 · 2025-04-04T07:32:59Z

Description

@Vipul-Cariappa and I were discussing a use case yesterday (especially while building with repl) and this is what we realized

The function responsible for creating clang::Interpreter is createClangInterpreter which can return a nullptr, for eg

CppInterOp/lib/Interpreter/Compatibility.h

Line 277 in c3384fb

return nullptr;
This is assigned to inner while the constructor for compat::Interpreter is called

CppInterOp/lib/Interpreter/CppInterOpInterpreter.h

Line 153 in c3384fb

inner = compat::createClangInterpreter(vargs);

which means inner can be nullptr

And then if we look at CreateInterpreter from the top, we're creating the compat::Interpreter here

CppInterOp/lib/Interpreter/CppInterOp.cpp

Line 2960 in c3384fb

auto I = new compat::Interpreter(ClingArgv.size(), &ClingArgv[0]);

Which means compat::Interpreter can exist even when the underlying clang::Interpreter is not existing. So we can say that the Interpreter from the top is half broken

So if we try something like

Cpp::CreateInterpreter(...)
Cpp::Parse(..)

We would hit the following finally

CppInterOp/lib/Interpreter/CppInterOpInterpreter.h

Lines 174 to 176 in c3384fb

    
           llvm::Expected<clang::PartialTranslationUnit&> Parse(llvm::StringRef Code) { 
        
             return inner->Parse(Code); 
        
           }

And although the call is made to compat::Interpreter, we end up using the underlying clang::Interpreter without checking if it is valid.

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

anutosh491 · 2025-04-04T07:37:55Z

Hence, we had a couple approaches to go ahead here

i) Introduce a isValid for clang::Interpreter and anytime we have a inner->func_call we use that (sounds a bit cumbersome checking for validity everytime)

ii) What Vipul and I liked was propagating the error all the way to that top, so that a half broken interpreter can't be put to use and a utility function like parse/declare would only work if CreateInterpreter doesn't return null along with an error message saying Interpreter creation failed.

codecov · 2025-04-04T07:43:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.16%. Comparing base (208c36a) to head (0268905).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   75.93%   76.16%   +0.23%     
==========================================
  Files           9        9              
  Lines        3644     3655      +11     
==========================================
+ Hits         2767     2784      +17     
+ Misses        877      871       -6

Files with missing lines	Coverage Δ
lib/Interpreter/CXCppInterOp.cpp	`48.86% <100.00%> (+1.58%)`	⬆️
lib/Interpreter/Compatibility.h	`91.96% <100.00%> (+1.87%)`	⬆️
lib/Interpreter/CppInterOp.cpp	`83.91% <100.00%> (+0.02%)`	⬆️
lib/Interpreter/CppInterOpInterpreter.h	`80.54% <100.00%> (+0.43%)`	⬆️

Files with missing lines	Coverage Δ
lib/Interpreter/CXCppInterOp.cpp	`48.86% <100.00%> (+1.58%)`	⬆️
lib/Interpreter/Compatibility.h	`91.96% <100.00%> (+1.87%)`	⬆️
lib/Interpreter/CppInterOp.cpp	`83.91% <100.00%> (+0.02%)`	⬆️
lib/Interpreter/CppInterOpInterpreter.h	`80.54% <100.00%> (+0.43%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

lib/Interpreter/CXCppInterOp.cpp

lib/Interpreter/CppInterOp.cpp

lib/Interpreter/CppInterOpInterpreter.h

lib/Interpreter/Compatibility.h

vgvassilev

Can we figure out how to write tests?

lib/Interpreter/CXCppInterOp.cpp

github-actions

clang-tidy made some suggestions

lib/Interpreter/CXCppInterOp.cpp

lib/Interpreter/CppInterOpInterpreter.h

github-actions

clang-tidy made some suggestions

lib/Interpreter/CXCppInterOp.cpp

unittests/CppInterOp/InterpreterTest.cpp

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/CUDATest.cpp

anutosh491 · 2025-04-09T13:22:18Z

Can we figure out how to write tests?

Okay I think the codecov says that 3 patches are not being tested. All of them are directly based out of the changes in CXCppInterOp.cpp

I tried adding tests but was quick to realize that I am not really doing just justice as I might not be the best one to goto for adding tests for the C API (hence I reverted them and squashed the commits 😅)

cc @Gnimuc

Would be great if you could help me out with the tests here. The PR description (#553 (comment)) should be good to get you started on the changed made in the C-API (the changes are not too concerning, simple ones !)

So we need to test clang_createInterpreter itself which is not being tested anywhere as per codecov

Maybe for the creation of the Intepreter we could start with something simple like

#ifndef CPPINTEROP_USE_CLING
TEST(InterpreterTest, CreateInterpreterCAPI) {
 const char* argv[] = {"-std=c++17"};
 CXInterpreter CXI = clang_createInterpreter(argv, 1);
 EXPECT_TRUE(CXI);


 clang_Interpreter_dispose(CXI);
}
#endif

For testing the failure while creating the interpreter (the case where we return nullptr from clang_createInterpreter), not sure what we can try. Maybe creating an non existent target or so I guess

#ifndef CPPINTEROP_USE_CLING
TEST(InterpreterTest, CreateInterpreterCAPIFailure) {
  // This should return nullptr due to bad target triple
  const char* argv[] = {"-target", "nonexistent-unknown-target"};
  CXInterpreter CXI = clang_createInterpreter(argv, 2);
  EXPECT_EQ(CXI, nullptr);
}
#endif

Thanks in advance ;)

Gnimuc · 2025-04-10T10:17:52Z

I'll give it a look this weekend.

anutosh491 · 2025-04-10T10:25:12Z

I'll give it a look this weekend.

Thanks a lot for the help. This change ends up being critical now that we'll soon be moving to llvm 20 !

Gnimuc · 2025-04-13T08:15:27Z

The C API clang_createInterpreter aims to provide a way to customize compiler arguments that are used to create the interpreter, so it doesn't adopt the following setups provided in the C++ API.

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 2924 to 2958 in 98a57b4

    
               ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str()); 
        
           #ifdef _WIN32 
        
               // FIXME : Workaround Sema::PushDeclContext assert on windows 
        
               ClingArgv.push_back("-fno-delayed-template-parsing"); 
        
           #endif 
        
               ClingArgv.insert(ClingArgv.end(), Args.begin(), Args.end()); 
        
               // To keep the Interpreter creation interface between cling and clang-repl 
        
               // to some extent compatible we should put Args and GpuArgs together. On the 
        
               // receiving end we should check for -xcuda to know. 
        
               if (!GpuArgs.empty()) { 
        
                 llvm::StringRef Arg0 = GpuArgs[0]; 
        
                 Arg0 = Arg0.trim().ltrim('-'); 
        
                 if (Arg0 != "cuda") { 
        
                   llvm::errs() << "[CreateInterpreter]: Make sure --cuda is passed as the" 
        
                                << " first argument of the GpuArgs\n"; 
        
                   return nullptr; 
        
                 } 
        
               } 
        
               ClingArgv.insert(ClingArgv.end(), GpuArgs.begin(), GpuArgs.end()); 
        
               // Process externally passed arguments if present. 
        
               std::vector<std::string> ExtraArgs; 
        
               auto EnvOpt = 
        
                   llvm::sys::Process::GetEnv("CPPINTEROP_EXTRA_INTERPRETER_ARGS"); 
        
               if (EnvOpt) { 
        
                 StringRef Env(*EnvOpt); 
        
                 while (!Env.empty()) { 
        
                   StringRef Arg; 
        
                   std::tie(Arg, Env) = Env.split(' '); 
        
                   ExtraArgs.push_back(Arg.str()); 
        
                 } 
        
               } 
        
               std::transform(ExtraArgs.begin(), ExtraArgs.end(), 
        
                              std::back_inserter(ClingArgv), 
        
                              [&](const std::string& str) { return str.c_str(); });

It's a bit tricky to set up a successful test environment because we need to know the path of system headers for each test platform. (Although most of the arguments are excluded in the C API, -include new is mandatory, which needs the correct path of system headers)

Gnimuc · 2025-04-13T08:21:00Z

Given auto* I = Cpp::CreateInterpreter();, if we can find a way to retrieve the arguments used to create the interpreter, then those arguments can be used to test the C API.

BTW, I think the changes in the PR look good.

anutosh491 · 2025-04-13T08:33:42Z

Thanks for the reply @Gnimuc

BTW, I think the changes in the PR look good.

Also thanks for going through the changes ! Much appreciated :)

Hey @vgvassilev , do you think it's okay if we move ahead with this ?

As said above there's some discussion that needs to be done on testing the C API. And although some simple dummy tests can possibly be added as I wrote above, those won't probably be meaningful enough. So yeah the changes introduce 4 lines of untested code in the C API but we can probably get to it in the future.

The main reason for this Pr is to help @mcbarton 's PR moving to llvm 20 (as his Pr should benefit with these changes after a rebase)

vgvassilev · 2025-04-13T09:16:44Z

The C API clang_createInterpreter aims to provide a way to customize compiler arguments that are used to create the interpreter, so it doesn't adopt the following setups provided in the C++ API.

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 2924 to 2958 in 98a57b4

ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());

#ifdef _WIN32

// FIXME : Workaround Sema::PushDeclContext assert on windows

ClingArgv.push_back("-fno-delayed-template-parsing");

#endif

ClingArgv.insert(ClingArgv.end(), Args.begin(), Args.end());

// To keep the Interpreter creation interface between cling and clang-repl

// to some extent compatible we should put Args and GpuArgs together. On the

// receiving end we should check for -xcuda to know.

if (!GpuArgs.empty()) {

llvm::StringRef Arg0 = GpuArgs[0];

Arg0 = Arg0.trim().ltrim('-');

if (Arg0 != "cuda") {

llvm::errs() << "[CreateInterpreter]: Make sure --cuda is passed as the"

<< " first argument of the GpuArgs\n";

return nullptr;

}

}

ClingArgv.insert(ClingArgv.end(), GpuArgs.begin(), GpuArgs.end());

// Process externally passed arguments if present.

std::vector<std::string> ExtraArgs;

auto EnvOpt =

llvm::sys::Process::GetEnv("CPPINTEROP_EXTRA_INTERPRETER_ARGS");

if (EnvOpt) {

StringRef Env(*EnvOpt);

while (!Env.empty()) {

StringRef Arg;

std::tie(Arg, Env) = Env.split(' ');

ExtraArgs.push_back(Arg.str());

}

}

std::transform(ExtraArgs.begin(), ExtraArgs.end(),

std::back_inserter(ClingArgv),

[&](const std::string& str) { return str.c_str(); });

It's a bit tricky to set up a successful test environment because we need to know the path of system headers for each test platform. (Although most of the arguments are excluded in the C API, -include new is mandatory, which needs the correct path of system headers)

Can we make the new header optional?

anutosh491 · 2025-04-13T12:35:36Z

Can we make the new header optional?

I think this should be possible isn't it !

The include new header would be required for tests like these

CppInterOp/unittests/CppInterOp/FunctionReflectionTest.cpp

Lines 1361 to 1375 in 98a57b4

    
           TEST(FunctionReflectionTest, JitCallAdvanced) { 
        
           #ifdef EMSCRIPTEN 
        
             GTEST_SKIP() << "Test fails for Emscipten builds"; 
        
           #endif 
        
             if (llvm::sys::RunningOnValgrind()) 
        
               GTEST_SKIP() << "XFAIL due to Valgrind report"; 
        
             std::vector<Decl*> Decls; 
        
             std::string code = R"( 
        
                 typedef struct _name { 
        
                   _name() { p[0] = (void*)0x1; p[1] = (void*)0x2; p[2] = (void*)0x3; } 
        
                   void* p[3]; 
        
                 } name; 
        
               )"; 
        
             GetAllTopLevelDecls(code, Decls);

Now on line 1375 we have the GetAllTopLevelDecls which ends up calling our CreateInterpreter

CppInterOp/unittests/CppInterOp/Utils.cpp

Lines 21 to 23 in 98a57b4

    
           void TestUtils::GetAllTopLevelDecls(const std::string& code, std::vector<Decl*>& Decls, 
        
                                               bool filter_implicitGenerated /* = false */) { 
        
             Cpp::CreateInterpreter();

So technically if we can improve GetAllTopLevelDecls to take care of custom args-> we can pass the same args to CreateInterpreter from there and whichever test needs whatever header include can use it. That way it is not mandatory to get that include for tests who wouldn't want to use it.

But yeah that would mean changing the function definition of the utility function GetAllTopLevelDecls overall :\
So probably should be done in subsequent PR !!! (which I can try tomorrow)

anutosh491 · 2025-04-13T13:06:24Z

Ahh the 3 failing checks are due to this

compiler-research/xeus-cpp#286

Making the same change here too ! I think we should be good after that !

mcbarton · 2025-04-13T17:13:30Z

Ahh the 3 failing checks are due to this

compiler-research/xeus-cpp#286

Making the same change here too ! I think we should be good after that !

I think this change should be made as a separate PR, and this PR rebased on it, given this pinning change has nothing to do with the goal of this PR. I will approve and merge this pinning PR, as soon as I see its been put in.

Can we make the new header optional?

I think this should be possible isn't it !

The include new header would be required for tests like these

CppInterOp/unittests/CppInterOp/FunctionReflectionTest.cpp

Lines 1361 to 1375 in 98a57b4

TEST(FunctionReflectionTest, JitCallAdvanced) {

#ifdef EMSCRIPTEN

GTEST_SKIP() << "Test fails for Emscipten builds";

#endif

if (llvm::sys::RunningOnValgrind())

GTEST_SKIP() << "XFAIL due to Valgrind report";

std::vector<Decl*> Decls;

std::string code = R"(

typedef struct _name {

_name() { p[0] = (void*)0x1; p[1] = (void*)0x2; p[2] = (void*)0x3; }

void* p[3];

} name;

)";

GetAllTopLevelDecls(code, Decls);

Now on line 1375 we have the GetAllTopLevelDecls which ends up calling our CreateInterpreter

CppInterOp/unittests/CppInterOp/Utils.cpp

Lines 21 to 23 in 98a57b4

void TestUtils::GetAllTopLevelDecls(const std::string& code, std::vector<Decl*>& Decls,

bool filter_implicitGenerated /* = false */) {

Cpp::CreateInterpreter();

So technically if we can improve GetAllTopLevelDecls to take care of custom args-> we can pass the same args to CreateInterpreter from there and whichever test needs whatever header include can use it. That way it is not mandatory to get that include for tests who wouldn't want to use it.

But yeah that would mean changing the function definition of the utility function GetAllTopLevelDecls overall :\ So probably should be done in subsequent PR !!! (which I can try tomorrow)

If making this header option would allow this PR to be tested, then should probably be done first, and then this PR should be rebased this on it. Otherwise we risk tests never been made to cover these lines. I know its just a few lines, but its because due to not insisting all lines be covered by tests, we have ended up in a situation where CppInterOps coverage has dropped by approximately 13% in the last 12 months (I made an issue about this here #541).

anutosh491 · 2025-04-13T17:45:46Z

Hi,

I think this change should be made as a separate PR

I've taken care of this here #559

If making this header option would allow this PR to be tested, then should probably be done first, and then this PR should be rebased this on it. Otherwise we risk tests never been made to cover these lines. I know its just a few lines, but its because due to not insisting all lines be covered by tests, we have ended up in a situation where CppInterOps coverage has dropped by approximately 13% in the last 12 months (I made an issue about this here #541).

Hmmm, I get it but shouldn't higher priority be moving to LLVM 20 now (it was out on FEB 2 and its high time we move) . The purpose of this PR was to help with that.

Also I am not a huge fan of nobody officially taking responsibility for covering the C API. It shouldn't have been added in the first place if that was the case !

Most of my focus since the past 2-3 weeks is around getting a good build (native/emscripten) for cppinterop around llvm 20 and technically this PR is also tested well on our C++ API. But for the C-API
i) I am not educated enough on the use case.
ii) I would really fall short on time to handle development/coverage there.

But this shouldn't slow any development on C++ API side is what I'm trying to convey.

I'm just going by Gnimuc 's word who added the C-API (he says "BTW, I think the changes in the PR look good." #553 (comment)) so I am just hoping we would be in a good enough state to test it in the near future. But if something as simple as a "create interpreter" function is not tested in the C API do we really stop other work due to that ? I don't know.

I just think handling error propagation while creating the interpreter is much more important that all of the above (but those might just be my views)

anutosh491 · 2025-04-14T05:43:03Z

If making this header option would allow this PR to be tested

I asked @kr-2003 to help me with this and we came up with this #561

Gnimuc · 2025-04-14T09:49:59Z

#ifndef CPPINTEROP_USE_CLING
TEST(InterpreterTest, CreateInterpreterCAPIFailure) {
// This should return nullptr due to bad target triple
const char* argv[] = {"-target", "nonexistent-unknown-target"};
CXInterpreter CXI = clang_createInterpreter(argv, 2);
EXPECT_EQ(CXI, nullptr);
}
#endif

@anutosh491 Maybe it's a good idea to add this failure test for now. It will improve the coverage a bit.

Gnimuc · 2025-04-14T09:54:12Z

The C API clang_createInterpreter aims to provide a way to customize compiler arguments that are used to create the interpreter, so it doesn't adopt the following setups provided in the C++ API.

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 2924 to 2958 in 98a57b4

ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());

#ifdef _WIN32

// FIXME : Workaround Sema::PushDeclContext assert on windows

ClingArgv.push_back("-fno-delayed-template-parsing");

#endif

ClingArgv.insert(ClingArgv.end(), Args.begin(), Args.end());

// To keep the Interpreter creation interface between cling and clang-repl

// to some extent compatible we should put Args and GpuArgs together. On the

// receiving end we should check for -xcuda to know.

if (!GpuArgs.empty()) {

llvm::StringRef Arg0 = GpuArgs[0];

Arg0 = Arg0.trim().ltrim('-');

if (Arg0 != "cuda") {

llvm::errs() << "[CreateInterpreter]: Make sure --cuda is passed as the"

<< " first argument of the GpuArgs\n";

return nullptr;

}

}

ClingArgv.insert(ClingArgv.end(), GpuArgs.begin(), GpuArgs.end());

// Process externally passed arguments if present.

std::vector<std::string> ExtraArgs;

auto EnvOpt =

llvm::sys::Process::GetEnv("CPPINTEROP_EXTRA_INTERPRETER_ARGS");

if (EnvOpt) {

StringRef Env(*EnvOpt);

while (!Env.empty()) {

StringRef Arg;

std::tie(Arg, Env) = Env.split(' ');

ExtraArgs.push_back(Arg.str());

}

}

std::transform(ExtraArgs.begin(), ExtraArgs.end(),

std::back_inserter(ClingArgv),

[&](const std::string& str) { return str.c_str(); });

It's a bit tricky to set up a successful test environment because we need to know the path of system headers for each test platform. (Although most of the arguments are excluded in the C API, -include new is mandatory, which needs the correct path of system headers)

Can we make the new header optional?

I'm wondering what's the original purpose of adding this header. Is it related to the hard-coded code in clang::Interpreter::create. An operator new trampoline function is declared there.

vgvassilev · 2025-04-14T11:06:11Z

The C API clang_createInterpreter aims to provide a way to customize compiler arguments that are used to create the interpreter, so it doesn't adopt the following setups provided in the C++ API.

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 2924 to 2958 in 98a57b4

ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());

#ifdef _WIN32

// FIXME : Workaround Sema::PushDeclContext assert on windows

ClingArgv.push_back("-fno-delayed-template-parsing");

#endif

ClingArgv.insert(ClingArgv.end(), Args.begin(), Args.end());

// To keep the Interpreter creation interface between cling and clang-repl

// to some extent compatible we should put Args and GpuArgs together. On the

// receiving end we should check for -xcuda to know.

if (!GpuArgs.empty()) {

llvm::StringRef Arg0 = GpuArgs[0];

Arg0 = Arg0.trim().ltrim('-');

if (Arg0 != "cuda") {

llvm::errs() << "[CreateInterpreter]: Make sure --cuda is passed as the"

<< " first argument of the GpuArgs\n";

return nullptr;

}

}

ClingArgv.insert(ClingArgv.end(), GpuArgs.begin(), GpuArgs.end());

// Process externally passed arguments if present.

std::vector<std::string> ExtraArgs;

auto EnvOpt =

llvm::sys::Process::GetEnv("CPPINTEROP_EXTRA_INTERPRETER_ARGS");

if (EnvOpt) {

StringRef Env(*EnvOpt);

while (!Env.empty()) {

StringRef Arg;

std::tie(Arg, Env) = Env.split(' ');

ExtraArgs.push_back(Arg.str());

}

}

std::transform(ExtraArgs.begin(), ExtraArgs.end(),

std::back_inserter(ClingArgv),

[&](const std::string& str) { return str.c_str(); });

It's a bit tricky to set up a successful test environment because we need to know the path of system headers for each test platform. (Although most of the arguments are excluded in the C API, -include new is mandatory, which needs the correct path of system headers)

Can we make the new header optional?

I'm wondering what's the original purpose of adding this header. Is it related to the hard-coded code in clang::Interpreter::create. An operator new trampoline function is declared there.

We did not have a trampoline in the past. I think maybe now we can drop it for llvm20?

anutosh491 · 2025-04-14T11:34:03Z

Hey @Gnimuc, we just dropped the header through #561

Does this put you in a better position to add some basic tests for clang_createInterpreter or other related functions ?

Would be great if yes, cause then we can have some initial set of tests like very simple ones. Move that in and then rebase this PR on top of those changes if that makes sense !

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp

mcbarton · 2025-04-15T07:55:21Z

unittests/CppInterOp/InterpreterTest.cpp

@@ -167,6 +167,22 @@ TEST(InterpreterTest, CreateInterpreter) {
 #endif
 }

+#ifndef CPPINTEROP_USE_CLING


Is there a reason why we are not doing this test for Cling?

No clue,

I see no C-API tests done through cling and that's what I am trying to replicate. Maybe @Gnimuc can educate us here.

The original purpose of adding the C API is to support multiple instances. REPL and Cling are two backends for creating compiler instances. REPL is based on clang::Interpreter from the upstream llvm-project.

I think there is no plan to upstream Cling to the llvm-project, and I didn't check Cling's compatibility when I added those C-API tests.

You can build, CppInterOp by providing cling and it will embed cling in it. If that’s not too much of work maybe we can support that, shouldn’t hurt.

Can we have a dedicated PR for that ?

I'll say the purpose of this PR is complete now.

If that’s not too much of work maybe we can support that, shouldn’t hurt.

@Gnimuc would you like to give this an attempt ?

anutosh491 · 2025-04-15T08:18:25Z

Hey @Gnimuc @vgvassilev

I have added some simple tests and ensured codecov doesn't complain about changes atleast with regard to this PR.

All tests pass no clue why the windows CI fail (don't look like a direct outcome of my tests)

Shall try looking into it for a bit (or shall skip If I am helpless cause I don't have a windows machine :\ )

kr-2003 · 2025-04-15T08:51:43Z

Hey @Gnimuc @vgvassilev

I have added some simple tests and ensured codecov doesn't complain about changes atleast with regard to this PR.

All tests pass no clue why the windows CI fail (don't look like a direct outcome of my tests)

Shall try looking into it for a bit (or shall skip If I am helpless cause I don't have a windows machine :\ )

Tests which write something on screen fails on Windows. Here, in this comment @mcbarton explained this:
#513 (comment)

mcbarton · 2025-04-15T08:56:46Z

Hey @Gnimuc @vgvassilev
I have added some simple tests and ensured codecov doesn't complain about changes atleast with regard to this PR.
All tests pass no clue why the windows CI fail (don't look like a direct outcome of my tests)

Shall try looking into it for a bit (or shall skip If I am helpless cause I don't have a windows machine :\ )

Tests which write something on screen fails on Windows. Here, in this comment @mcbarton explained this: #513 (comment)

To refine this comment from before. I think anything which makes the interpreter write an error message in Windows, will be picked up as an error in the overall running of the tests. Tests which seem to write a warning message are ok. I believe it is ok for you skip this test on Windows for this reason.

kr-2003 · 2025-04-15T09:01:59Z

To refine this comment from before. I think anything which makes the interpreter write an error message in Windows, will be picked up as an error in the overall running of the tests. Tests which seem to write a warning message are ok. I believe it is ok for you skip this test on Windows for this reason.

Alternatively, we can suppress the error message and retain this test on windows too. It should pass now.


TEST(InterpreterTest, CreateInterpreterCAPIFailure) {
#ifdef CPPINTEROP_USE_CLING
  GTEST_SKIP() << "C API is not available in this build";
#endif
  const char* argv[] = {"-fsyntax-only", "-Xclang", "-invalid-plugin"};
  testing::internal::CaptureStderr(); // Suppress error messages
  auto *CXI = clang_createInterpreter(argv, 3);
  std::string capturedStderr = testing::internal::GetCapturedStderr();
  EXPECT_EQ(CXI, nullptr);
}

mcbarton · 2025-04-15T09:08:10Z

To refine this comment from before. I think anything which makes the interpreter write an error message in Windows, will be picked up as an error in the overall running of the tests. Tests which seem to write a warning message are ok. I believe it is ok for you skip this test on Windows for this reason.

Alternatively, we can suppress the error message and retain this test on windows too. It should pass now.
TEST(InterpreterTest, CreateInterpreterCAPIFailure) {
#ifdef CPPINTEROP_USE_CLING
  GTEST_SKIP() << "C API is not available in this build";
#endif
  const char* argv[] = {"-fsyntax-only", "-Xclang", "-invalid-plugin"};
  testing::internal::CaptureStderr(); // Suppress error messages
  auto *CXI = clang_createInterpreter(argv, 3);
  std::string capturedStderr = testing::internal::GetCapturedStderr();
  EXPECT_EQ(CXI, nullptr);
}

Not entirely sure testing::internal::CaptureStderr() will work to resolve this issue. Its probably better to skip for now on Windows. We do however want to make use of GTEST_SKIP(), instead of the way its done currently in the PR.

anutosh491 · 2025-04-15T10:10:26Z

Its probably better to skip for now on Windows

Yeah I think we can just skip it for now.

anutosh491 · 2025-04-15T10:47:16Z

I think this is ready from my side now that all the checks pass.

vgvassilev

Please also address the missing includes suggested by clang-tidy before merging.

unittests/CppInterOp/CUDATest.cpp

anutosh491 · 2025-04-17T11:04:08Z

Addressed all reviews. I think this should be ready now. Thanks everyone for the reviews. Squashing and Merging!

- update source to test compiler-research/CppInterOp#553 - remove wasm link flags in the cmake patch

anutosh491 changed the title ~~Fixing creation of Interpreter~~ Add error propagation to interpreter creation logic Apr 4, 2025

mcbarton requested a review from vgvassilev April 4, 2025 07:37

mcbarton requested a review from aaronj0 April 4, 2025 07:40

github-actions bot reviewed Apr 4, 2025

View reviewed changes

anutosh491 commented Apr 4, 2025

View reviewed changes

lib/Interpreter/Compatibility.h Show resolved Hide resolved

anutosh491 force-pushed the fix_inner branch from 502ac89 to c98a734 Compare April 4, 2025 08:13

vgvassilev approved these changes Apr 9, 2025

View reviewed changes

lib/Interpreter/CXCppInterOp.cpp Outdated Show resolved Hide resolved

anutosh491 force-pushed the fix_inner branch from c98a734 to 00192ed Compare April 9, 2025 10:47

github-actions bot reviewed Apr 9, 2025

View reviewed changes

lib/Interpreter/CXCppInterOp.cpp Outdated Show resolved Hide resolved

lib/Interpreter/CppInterOpInterpreter.h Show resolved Hide resolved

github-actions bot reviewed Apr 9, 2025

View reviewed changes

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Apr 9, 2025

View reviewed changes

anutosh491 force-pushed the fix_inner branch 2 times, most recently from 1bb5c43 to 764d551 Compare April 9, 2025 13:13

anutosh491 force-pushed the fix_inner branch from 764d551 to f8cb3ec Compare April 13, 2025 12:35

Fixing creation of Interpreter

fb47552

anutosh491 force-pushed the fix_inner branch from eb0116d to fb47552 Compare April 13, 2025 17:56

Merge branch 'main' into fix_inner

536c896

Add tests to check creation of interpreter for the C-API

b0370a0

github-actions bot reviewed Apr 15, 2025

View reviewed changes

unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved

unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved

unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved

mcbarton reviewed Apr 15, 2025

View reviewed changes

Skip for windows

f495d29

anutosh491 requested review from mcbarton and vgvassilev April 15, 2025 10:47

vgvassilev approved these changes Apr 17, 2025

View reviewed changes

unittests/CppInterOp/CUDATest.cpp Outdated Show resolved Hide resolved

anutosh491 added 3 commits April 17, 2025 15:22

addressed requested changes

768af5a

Merge branch 'main' into fix_inner

4ee86d0

Update CppInterOpInterpreter.h

0268905

anutosh491 merged commit 721d33b into compiler-research:main Apr 17, 2025
75 checks passed

anutosh491 deleted the fix_inner branch April 17, 2025 11:04

Gnimuc added a commit to Gnimuc/Yggdrasil that referenced this pull request Apr 19, 2025

libCppInterOp: build v0.1.7

f03db67

- update source to test compiler-research/CppInterOp#553 - remove wasm link flags in the cmake patch

Gnimuc mentioned this pull request Apr 19, 2025

libCppInterOp: build v0.1.7 JuliaPackaging/Yggdrasil#11045

Merged

giordano pushed a commit to JuliaPackaging/Yggdrasil that referenced this pull request Apr 22, 2025

libCppInterOp: build v0.1.7 (#11045)

90fc877

- update source to test compiler-research/CppInterOp#553 - remove wasm link flags in the cmake patch

kr-2003 mentioned this pull request May 13, 2025

Discussing CUDA support for xeus-cpp compiler-research/xeus-cpp#300

Open

6 tasks

	llvm::Expected<clang::PartialTranslationUnit&> Parse(llvm::StringRef Code) {
	return inner->Parse(Code);
	}

Add error propagation to interpreter creation logic #553

Add error propagation to interpreter creation logic #553

Uh oh!

Conversation

anutosh491 commented Apr 4, 2025

Description

Type of change

Testing

Checklist

Uh oh!

anutosh491 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anutosh491 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gnimuc commented Apr 10, 2025

Uh oh!

anutosh491 commented Apr 10, 2025

Uh oh!

Gnimuc commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gnimuc commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 commented Apr 4, 2025 •

edited

Loading

codecov bot commented Apr 4, 2025 •

edited

Loading

anutosh491 commented Apr 9, 2025 •

edited

Loading

Gnimuc commented Apr 13, 2025 •

edited

Loading

Gnimuc commented Apr 13, 2025 •

edited

Loading

anutosh491 commented Apr 13, 2025 •

edited

Loading

anutosh491 commented Apr 13, 2025 •

edited

Loading

mcbarton commented Apr 13, 2025 •

edited

Loading

anutosh491 commented Apr 13, 2025 •

edited

Loading

Gnimuc commented Apr 14, 2025 •

edited

Loading

anutosh491 commented Apr 14, 2025 •

edited

Loading

mcbarton commented Apr 15, 2025 •

edited

Loading

kr-2003 commented Apr 15, 2025 •

edited

Loading

mcbarton commented Apr 15, 2025 •

edited

Loading