Page MenuHomePhabricator

[clang-repl] Introduce Value to capture expression results
Needs ReviewPublic

Authored by junaire on Jan 7 2023, 10:58 PM.

Details

Reviewers
v.g.vassilev
Summary

This is the first part of the RFC, which implements the initial
infrastructure used for capturing expression results.

To capture the expression, it also adds a new annotation token called
annot_input_end, which works somewhat like the annot_end_of_module token
we already have. The idea is proposed by Richard Smith, thanks!

RFC: https://discourse.llvm.org/t/rfc-handle-execution-results-in-clang-repl/68493

Signed-off-by: Jun Zhang <jun@junz.org>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
junaire updated this revision to Diff 499801.Feb 23 2023, 3:54 AM

Update tests.

junaire updated this revision to Diff 499804.Feb 23 2023, 3:59 AM

Rename the interface.

junaire updated this revision to Diff 501360.Feb 28 2023, 7:34 PM

Support printing temporaries.

Note there's a ugly hack because I didn't find the right way to synthesize
the AST for explicit destructor call.

junaire retitled this revision from [clang-repl][WIP] Implement pretty printing to [clang-repl] Introduce Value and implement pretty printing.Feb 28 2023, 7:38 PM
junaire edited the summary of this revision. (Show Details)
junaire updated this revision to Diff 501363.Feb 28 2023, 7:48 PM
junaire edited the summary of this revision. (Show Details)

Remove extra includes

junaire updated this revision to Diff 501365.Feb 28 2023, 7:59 PM

Add more tests

junaire updated this revision to Diff 502405.Sat, Mar 4, 10:30 PM

Prefer one patch

junaire updated this revision to Diff 502406.Sat, Mar 4, 10:34 PM

Fix patch application failed

junaire retitled this revision from [clang-repl] Introduce Value and implement pretty printing to [clang-repl] Introduce Value to capture expression results.Fri, Mar 24, 6:28 AM
junaire edited the summary of this revision. (Show Details)
junaire updated this revision to Diff 508072.Fri, Mar 24, 6:34 AM

Update comments

junaire updated this revision to Diff 509305.Wed, Mar 29, 4:34 AM

Update + Rebase

Overall it looks like we are heading in a good direction. Here are some initial comments from my partial review.

clang/include/clang/Interpreter/Interpreter.h
107

Do we need to expose this function to the outside world?

clang/lib/Interpreter/ASTHelpers.cpp
1 ↗(On Diff #509311)

Likewise.

clang/lib/Interpreter/ASTHelpers.h
1 ↗(On Diff #509311)

We are missing the standard llvm header preamble. Clang seems to have slight preference to using Utils in the name. How about renaming this file to InterpreterUtils.h

clang/lib/Interpreter/IncrementalParser.cpp
22

This introduces a layering violation. Can we avoid it?

161

Why ConsumeToken is not sufficient for an annot_input_end?

clang/lib/Interpreter/IncrementalParser.h
82

Do we need to expose CodeGen in this patch?

junaire updated this revision to Diff 509958.Fri, Mar 31, 3:18 AM

Address comments.

junaire marked 3 inline comments as done.Fri, Mar 31, 3:22 AM
junaire added inline comments.
clang/lib/Interpreter/IncrementalParser.cpp
161
v.g.vassilev added inline comments.Fri, Mar 31, 5:01 AM
clang/include/clang/Interpreter/Interpreter.h
20

We can forward declare Value.

38

We probably do not need these forward declarations.

99

This seems to be not needed as well.

104

Maybe these can go in a separate data structure called ValuePrintingInfo or similar.

117

We should not need this interface.

clang/lib/Interpreter/IncrementalParser.cpp
161

Ok, thanks!

clang/lib/Interpreter/Interpreter.cpp
37

This seems to be pointing to the old filename.

226

We should avoid cantFail here and propagate the error as we do a few lines above.

clang/lib/Interpreter/Value.cpp
2

We are missing the llvm preamble here as well.

clang/lib/Parse/ParseStmt.cpp
551

I think the cleaner solution here is to remove the global flag and pass it handleExprStmt and extend Sema::ActOnExprStmt.

junaire updated this revision to Diff 510014.Fri, Mar 31, 6:41 AM

Address more comments

junaire marked 9 inline comments as done.Fri, Mar 31, 6:44 AM
junaire added inline comments.
clang/include/clang/Interpreter/Interpreter.h
117

Interpreter::CompileDecl needs it. So we don't have to do a dummy Interp.Parse("") to get a llvm::Module.

clang/lib/Parse/ParseStmt.cpp
551

I dont understand, can you elaborate a bit?

junaire updated this revision to Diff 510226.Sat, Apr 1, 7:42 AM

I dont know why the premerge tests are failling, let me try to do a update...