[Lldb-commits] [PATCH] D68188: allow arbitrary python streams to be converted to SBFile

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, I think that the major issues are out of the way now. I've done a more careful pass over the file, and I think this is looking pretty good. I am particularly happy with how you've implemented (and tested) the interaction with python exceptions. A bunch of additional comments inline, but none really that important.



================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:347-348
+
+    @add_test_categories(['pyapi'])
+    @no_debug_info_test
+    def test_sbfile_write_borrowed(self):
----------------
Actually, I'm pretty sure that this category annotation is unnecessary, as the `python_api` contains a `.categories` file specifying this category. Also, the `@no_debug_info_test` thingy might be better achieved by setting `NO_DEBUG_INFO_TESTCASE = True` class property.


================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:477-478
+            status = self.debugger.SetOutputFile(sbf)
+            if status.Fail():
+                raise Exception(status)
+            self.handleCmd('script 2+2')
----------------
`self.assertTrue(status.Succes())`. If you're doing this because you want a better error message or something, feel free to implement something like `assertSuccess` or the like. I'd be happy to use that, as I want better error messages too.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1039
 
+class GIL {
+public:
----------------
All of these file-local classes (unless you choose to move them to a separate file) should go into an anonymous namespace. See <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces> on the recommended way to do that.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1069-1071
+  if (log) {
+    log->Printf("%s failed with exception: %s", caller, toCString());
+  }
----------------
We're using LLDB_LOG or LLDB_LOGF these days. The first one is prefereed if you have complex values that can't be displayed by printf natively (e.g. StringRef).


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1176
+
+// A SimplyPythonFile is a OwnedPythonFile that just does all I/O as
+// a NativeFile
----------------
s/Simply/Simple


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1180-1182
+  SimplePythonFile(int fd, uint32_t options, const PythonFile &file,
+                   bool borrowed)
+      : OwnedPythonFile(file, borrowed, fd, options, false){};
----------------
This reordering of arguments is confusing. Can we make the two orders match?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1238-1245
+// OwnedPythonFile<Base>::IsValid() chains into Base::IsValid()
+// File::IsValid() is false by default, but for the following classes
+// we want the file to be considered valid as long as the python bits
+// are valid.
+class PresumptivelyValidFile : public File {
+public:
+  bool IsValid() const override { return true; };
----------------
How about if OwnedPythonFile defines `IsValid()` as `IsPythonObjectValid() || Base::IsValid()`. Then PythonIOFile could redefine it to be simply `IsPythonObjectValid()` without the need for the extra class?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1251
+  PythonIOFile(const PythonFile &file, bool borrowed)
+      : OwnedPythonFile(file, borrowed){};
+
----------------
no semicolon


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1255-1264
+  Status Close() override {
+    assert(m_py_obj);
+    GIL takeGIL;
+    if (m_borrowed)
+      return Flush();
+    Take<PythonObject>(PyObject_CallMethod(m_py_obj, "close", "()"));
+    if (PyErr_Occurred())
----------------
Hmm... how exactly does this differ from the implementation in `OwnedPythonFile`? Is it only that it does not chain into the base class? Could we use the same trick as for IsValid to reduce the duplication?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1278
+class BinaryPythonFile : public PythonIOFile {
+  friend class PythonFile;
+
----------------
What is this needed for? I don't see the PythonFile class doing anything funny (and it probably shouldn't).


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1397
+
+llvm::Expected<FileSP> PythonFile::ConvertToFileForcingUseOfScriptingIOMethods(
+    bool borrowed) {
----------------
I was struggling to find this method, as I was looking it next to the other PythonFile methods. I get why it's here, but I think it's confusing to have PythonFile be split this way. Could you move the other classes so that they are declared before all of PythonFile methods. Or, given that this file is starting to become largish, maybe move some of this stuff into a separate file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68188