sgraenitz added a comment. I think this makes good progress. I found two details in the test code that need attention. The stdout issue might be done now or in a follow-up patch some time soon. Otherwise, this seems to get ready to land.
@teemperor What about your notes. Are there any open issues remaining? ================ Comment at: clang/lib/Interpreter/Interpreter.cpp:93 + "Initialization failed. " + "Unable to create diagnostics engine"); + ---------------- It looks like `clang-repl` always dumps errors to stdout currently. This is fine for the interactive use case, but it will be impractical for input/output tests. As a result unit tests e.g. dump: ``` Note: Google Test filter = InterpreterTest.Errors [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from InterpreterTest [ RUN ] InterpreterTest.Errors In file included from <built-in>:0: input_line_0:1:1: error: unknown type name 'intentional_error' intentional_error v1 = 42; ^ [ OK ] InterpreterTest.Errors (9024 ms) [----------] 1 test from InterpreterTest (9024 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (9025 ms total) [ PASSED ] 1 test. ``` It would be useful to have an option for streaming diagnostics to an in-memory buffer (and maybe append them to returned llvm::Error instances in the future). Instead of `createDiagnostics()` you could pass a TextDiagnosticPrinter via `setDiagnostics()` here to accomplish it. Not insisting on having it in this review, but it would be a good follow-up task at least. ================ Comment at: clang/test/Interpreter/execute.c:9 + +struct S { float f = 1.0; S *m = nullptr;} s; +auto r2 = printf("S[f=%f, m=%p]\n", s.f, s.m); ---------------- *nit* this should be a cpp file right? Otherwise, you should write `struct S *m = nullptr;` here. Also, C doesn't understand the `extern "C"` above :) ================ Comment at: clang/test/Interpreter/execute.c:11 +auto r2 = printf("S[f=%f, m=%p]\n", s.f, s.m); +// CHECK-NEXT: S[f=1.000000, m=(nil)] + ---------------- The `%p` format placeholder in printf is implementation-defined, so the output here varies. Maybe you can do something like this instead: ``` auto r2 = printf("S[f=%f, m=0x%llx]\n", s.f, (unsigned long long)s.m); // CHECK-NEXT: S[f=1.000000, m=(0x0)] ``` Or reinterpret_cast<unsigned long long>(s.m) if you go the C++ way. ================ Comment at: clang/test/lit.cfg.py:105 + config.available_features.add('host-supports-jit') + if config.clang_staticanalyzer: ---------------- I couldn't test this on a host that doesn't support JIT, but it looks like a nice "duck typing" way of testing for the feature. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits