[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 30 16:42:45 PDT 2021
rsmith added inline comments.
================
Comment at: clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp:73-76
+#ifdef _MSC_VER
+// Tell the windows linker to export the type_info symbol required by exceptions
+#pragma comment(linker, "/export:??_7type_info@@6B@")
+#endif // _MSC_VER
----------------
Presumably this is redundant now we don't run the test on Windows?
================
Comment at: clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp:101-105
+ // FIXME: Somehow when we build this test in release mode argc is not 0.
+ // printf("%d\n", argc);
+ // for (int I = 0; I < argc; ++I)
+ // printf("arg[%d]='%s'\n", I, argv[I]);
+
----------------
Isn't this because you're passing no arguments to `main` when you call it later in the test? You're not passing any arguments on line 123/124.
================
Comment at: clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp:123-124
+ testing::internal::CaptureStdout();
+ auto Main = (int (*)(...))llvm::cantFail(Interp->getSymbolAddress("main"));
+ EXPECT_THROW(Main(), std::exception);
+ std::string CapturedStdOut = testing::internal::GetCapturedStdout();
----------------
I think this should probably be cast to `int(*)(int, const char**)` instead. But the name `main` is special, and might not have its declared type, so selecting a different function name would have less risk of weird behavior. I'd also suggest you remove the parameters if you're not going to use them.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107049/new/
https://reviews.llvm.org/D107049
More information about the cfe-commits
mailing list