This is an archive of the discontinued LLVM Phabricator instance.

[llgo] llgoi: separate evaluation from printing
ClosedPublic

Authored by axw on Oct 14 2015, 7:18 PM.

Details

Summary

Separate the evaluation of expressions from printing
of results. This is in preparation for splitting the
core of the interpreter out for use in alternative
interpreter frontends.

At the same time, the output is made less noisy in
response to comments on the golang-nuts announcement.
We would ideally print out values using Go syntax,
but this is impractical until we have libgo based on
Go 1.5. When that happens, fmt's %#v will handle
reflect.Value better, and so we can fix/filter type
names to remove automatically generated package names.

Diff Detail

Event Timeline

axw updated this revision to Diff 37441.Oct 14 2015, 7:18 PM
axw retitled this revision from to [llgo] llgoi: separate evaluation from printing.
axw updated this object.
axw added a reviewer: pcc.
axw added a subscriber: llvm-commits.
axw updated this revision to Diff 37694.Oct 17 2015, 8:04 PM

Revert to using NamedGlobal/PointerToGlobal

Unnecessary change that would require a change to bindings.

pcc added inline comments.Nov 4 2015, 1:38 PM
cmd/llgoi/llgoi.go
54

I normally just hacked this in manually whenever I needed it. Would that not work for you?

Nit: can the variable be named like the flag name?

182

Wouldn't this return null if the source package isn't a interpretLine-generated package? How do things work in that case?

200

I don't much like this conspiracy between loadSourcePackage and interpretLine. Can we maybe have this function return getPackageSymbol instead of result and make interpretLine responsible for retrieving the results?

531

I was working on a pretty printing package for this, mainly because I wanted more whitespace than what %#v provides, but I suppose it could also be used to hide internal names. If that sounds useful at all I'll see if I can dig it up.

test/llgoi/maps.test
10

Here (and in a few other places) you are removing CHECK lines instead of replacing them with something equivalent. In this particular case the expression line isn't really testing much any more (not even the parser/type checker, as you aren't testing for absence of errors). I'd prefer to keep the checks unless you have a reason not to, or otherwise remove lines like this.

axw updated this revision to Diff 54619.Apr 22 2016, 3:01 AM
axw marked 2 inline comments as done.

Addressed review comments

  • removed source printing flag
  • moved __llgoiResults extraction to interpretLine
  • replaced existing CHECKs in lit tests
axw added a comment.Apr 22 2016, 3:01 AM

@pcc PTAL. Sorry for leaving this for so long.

cmd/llgoi/llgoi.go
54

I'll back this out for now.

182

Fixed to return nil if the symbol isn't found. Should never happen now, anyway, since the __llgoiResults call has been moved into interpretLine

531

If you still have this, please do send it along. If not, I'll do it later.

pcc accepted this revision.Apr 24 2016, 1:44 PM
pcc edited edge metadata.

LGTM

cmd/llgoi/llgoi.go
531

I've uploaded the code I wrote to D19473. It may not be very useful though as I didn't get very far.

This revision is now accepted and ready to land.Apr 24 2016, 1:44 PM
axw closed this revision.Apr 24 2016, 6:24 PM