[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

This is a little bit odd, but it does make it easy to call methods on a value you got from FindVariable without having to cons up an expression.  That seems worthwhile.

It would be good to add a little more explanation of what this is doing in the docs: "in the context of the object" is a little vague.

You should add some tests where the operation doesn't make sense, for instance what if the SBValue is a scalar?  Or what if it is an expression result so it's not in memory.  You have some error handling in the patch but your test doesn't exercise it.  Be good to make sure that actually works.

You also only test the case where the value is a struct, can you test a pointer to a struct and an ObjC class?  They both should work but you don't have tests for them.

As an implementation detail, the fact that lldb has to get local variable lookup right by textually injecting the local variables declarations into the expression is a bug, these should be provided on demand from the ASTSource.  But that doesn't work correctly at present - the AST source gets asked AFTER looking in the class context which is to late - and we have do the local variable insertion trick instead.  So if we ever fix the bug in clang lookup, your implementation will break, and you will have to interfere in that lookup instead.  But that's for the future.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55318/new/

https://reviews.llvm.org/D55318