This is an archive of the discontinued LLVM Phabricator instance.

Initial version of libInterpreter.
AbandonedPublic

Authored by v.g.vassilev on Jan 11 2017, 6:46 AM.

Details

Reviewers
spyffe
rsmith
Summary

Hey, as we discussed in private I started working towards moving some code from cling to clang.

This is the very minimal patch (barely functional) which I intend to propose publicly.

I'd be happy to address your comments first and then send this to cfe-commits.

Diff Detail

Event Timeline

v.g.vassilev retitled this revision from to Initial version of libInterpreter..
v.g.vassilev updated this object.
v.g.vassilev added reviewers: rsmith, spyffe.

clang-format

spyffe requested changes to this revision.Jan 11 2017, 11:02 AM
spyffe edited edge metadata.

Very exciting, thank you for this patch!
I have a couple of top-level comments first, since I'm still getting familiar with this code.
The primary concern I have is the exit; secondary concerns are composability with other tools.

lib/Interpreter/IncrementalParser.cpp
88

If the incremental parser is no longer usable, that should be communicated out to the client rather than exiting.

144

It looks like we should be sharing this between the incremental interpreter and clang-import-test.

lib/Interpreter/IncrementalParser.h
46

Could this be an accessor, given that Consumer can always be derived from CI or Act?

55

It's not clear to me whether your code is going to rely on Consumer to do anything important, or whether it's just a no-op with the intent that the consumer of Parse's return value actually does the work. Can you clarify this relationship?

If Consumer is a no-op, then it's fine being opaque.
If it is going to be doing work, then at least from LLDB's perspective I would like to be able to provide my own.

58

Does getCI() need to be public? I know it's const, but it does seem to leak the abstraction that IncrementalParser is providing.

This revision now requires changes to proceed.Jan 11 2017, 11:02 AM
rsmith added inline comments.Jan 11 2017, 2:13 PM
lib/Interpreter/IncrementalParser.cpp
290–312

Can you factor this out into perhaps a method on Parser, along with the corresponding code in clang::ParseAST?

354

This is wrong: LLVM_ON_WIN32 is about the build environment where Clang is compiled, not about the target. The relevant consideration is likely the DelayedTemplateParsing LangOption.

354–363

This sounds like a bug. What tokens specifically are being left behind here?

367

It seems dangerous to lex an extra token in asserts mode. Maybe lex that final token unconditionally? (Given that we expect to have already lexed an EOF token here, lexing another one should be very cheap, so the #ifndef NDEBUG seems unnecessary.)

rsmith added inline comments.Jan 11 2017, 2:17 PM
lib/Interpreter/IncrementalParser.cpp
104

Creating this should be the responsibility of the cling driver binary; it's not appropriate for this library to choose to write textual diagnostics to stderr. (Consider the needs of a REPL in an IDE for instance.)

113

Likewise, this piece should be handled by the driver. This library should not assume any relationship between argv[0] and the compiler's resource directory.

120–123

Setting this up should be the responsibility of the cling driver binary, not of this library. This library should not be setting global state.

v.g.vassilev added inline comments.Jan 11 2017, 2:25 PM
lib/Interpreter/IncrementalParser.cpp
88

This is copy from the driver. We can fix this by introducing some sort of callback layer.

144

Absolutely. This code is very similar to what is done in runToolOnCodeWithArgs from tooling library (see one of the FIXMEs). Maybe we could reuse it even more there.

lib/Interpreter/IncrementalParser.h
55

Cling caches the produced top level decls before piping them to codegen. We do that for cases like int i = 42; error_here; int j; We do not want to produce code for int i = 42 if the input overall had errors.

This consumer is our special error recovery handler (keeping track of the state changes in the compiler). I can imagine that something similar could be used by lldb to make some parts of the AST visible when switching between frames.

I intentionally kept it as simple as possible. I can add accessors to it on demand.

58

You are right, giving access to the CompilerInstance though is our current point of end user customization.

Based on this leaking abstraction I will start playing with clang-import-test to figure out how much code could be reused.

v.g.vassilev abandoned this revision.Jun 27 2021, 10:31 PM

Version of this work landed as https://reviews.llvm.org/D96033