jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
Modifying the target environment in order to add something to the host lldb's
PYTHONPATH seems very counterintuitive to me. It is misusing the purpose of
the target environment setting. And it could cause confusion if you set this
up (say in your .lldbinit) and then months later wondered why some Python
program you were debugging ended up finding python modules in weird places...
If you want to have a way to add to lldb's python interpreter's path when
starting lldb, then there should be a setting for that specific purpose.
Also, this would need some sort of documention. There's no way anybody could
discover this feature without reading the lldb sources.
Also also, if I put:
script import sys; sys.path.append("/some/directory")
in my .lldbinit, wouldn't that do the same thing your patch does, but in a much
more straightforward way? I don't see the advantage of doing this in a
setting, and certainly not in the target env-vars setting.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:3280
+ PyRun_SimpleString("import os");
+ for (const auto &KV : env) {
+ if (strcmp(KV.getKey().data(), "PYTHONPATH") == 0 ||
----------------
lldb local variables should always be lower case, and should be words. I'm
assuming the this is called KV because it's a key/value entry, but it took me
too much time to guess that. "key_value" or something would make this easier to
read.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96370/new/
https://reviews.llvm.org/D96370
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
