labath added a comment.

I'm going to throw one more opinion into the mix here. :)

While I agree that sprinkling `#ifdef PYTHON` everywhere is bad, and it's great 
to be rid of it, I also think that it is good for the user to be able to know 
how a particular build of lldb was configured. So, I wouldn't say this goes 
"against the  notion of a generic pluggable interpreter". In fact I would say 
the opposite. In an imaginary future where lldb can be built with either python 
or say javascript support (or both?), it may be very important for programs to 
detect whether the library they are using is compatible with them.

The question on my mind is "do we ever want/anticipate the python path to 
change at runtime?". If we don't, then getting the path value through a command 
interpreter *instance* seems odd. In fact, I would say that the SBHostOS is the 
best place to provide this info, as it is really a property of the host, which 
does not depend on any runtime configuration of anything. So, if the only  
reason you made this go through the command interpreter class was to break the 
build dependency, then I think we should do this another way. One way to handle 
this would be to add one more argument to the PluginManager::RegisterPlugin 
function which is called from ScriptInterpreterPython::Initialize. This 
argument could be either a callback which computes the "module path" for the 
given language, or even the path itself. Then SBHostOS could just get the path 
by asking the PluginManager.

So, with both backward and forward compatibility in mind, what I'd to is this:

- don't add any new SB interfaces
- implement `SBHostOS::GetLLDBPythonPath` via a call to 
`PluginManager::GetModulePathFor(eScriptLanguagePython)` as described previously
- In the future, once multiple interpreters become a reality, consider a new 
SBHostOS function, which takes a language type as an argument. This will allow 
the user to determine which scripting languages are available and leave the 
door open for a single build of lldb to support multiple languages (which the 
current proposal doesn't). `SBHostOS::GetLLDBPythonPath` then just becomes a 
deprecated variant of this new function.


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

https://reviews.llvm.org/D61090



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

Reply via email to