Sorry, a few more quibbles.
Why not add FileSpec::GetFilenameStrippingExtension or something like that as
well, since that's what you really use? That seems like another useful
function. Don't take out the extension one, that also seems good.
What a pain that you can't import a Python module w/o adding its containing
directory to the Python Path! There must be some reason why, but sigh...
Why do you have to do this bit:
+ FILE *tmp_fh = (python_interpreter->m_dbg_stdout ?
python_interpreter->m_dbg_stdout : stdout);
+
+ {
+ Locker py_lock(python_interpreter, tmp_fh);
That seems non-obvious and at least merits a comment.
Finally since there seem to be several reasons why LoadScriptingModule could
fail in this function, it would be better if it took an Error & parameter and
filled that out appropriately.
Jim
On Oct 14, 2011, at 12:55 PM, Enrico Granata wrote:
> Hi all,
> hopefully the third time is the charm :)
>
>> The only quibble I have is that you do a bunch of path manipulation by hand
>> here that should be done through FileSpec.
>
>> Also, instead of pulling off the .pyc or .py extension by hand it would be
>> good to add this functionality to FileSpec, since that is also generally
>> useful.
>
> I have modified the implementation following Jim's advice, and now I use
> FileSpec directly. I have also added a method named GetFilenameExtension() to
> said class.
>
>> As to the test case, no, it does not belong in the python_api subdirectory.
>> You can create a
>> functionalities/command_script subdir to house the import dir.
>
> It looks like you had already created functionalities/command_script. I have
> created an import subdirectory there and moved my test case files there.
>
> Thanks for the feedback on this patch.
>
> Sincerely,
> <importcmd.diff>
> - Enrico Granata
_______________________________________________
lldb-dev mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
