[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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