[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Stefan Gränitz via Phabricator via cfe-commits]

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