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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to