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

Adding an SB API to set the target's search paths seems fine to me.  I think in 
theory the totality of LLDB functionality should be available through the SB 
API's, (a) because it is always awkward to fall back on HandleCommand and (b) 
because at some point we really should make the command-line be a fairly simple 
client of the SB API's as that would greatly reduce our testing surface.  We're 
only going to get there one bit at a time.

But this seems the wrong way to do it.  You are setting a property on an 
SBTarget so this should be a method on the SBTarget.  The selected target is 
really not reliable.  For instance, having this only available for the selected 
target would make this impossible to use in a breakpoint command, since you 
could have two running targets, target A could be selected but target B could 
hit a breakpoint.  We won't select B till we've decided that B is actually 
going to stop - which you don't know when you are executing breakpoint 
commands.  So API like this should really not rely on the selected target.  
BTW, I see this is right below SBDebugger::SetCurrentPlatformSDKRoot, but I 
don't approve of that one either...

Also, if you're going to the trouble of adding a "AddSharedObjectPath, you 
should also add ListSharedObjectPaths, and that will make it easier to write a 
test for this command.


Repository:
  rL LLVM

https://reviews.llvm.org/D47302



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

Reply via email to