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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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; };
----------------
lawrence_danna wrote:
> labath wrote:
> > 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?
> If I did that then a SimplePythonFile would still be valid if the file was closed on the python side... seems like the wrong behavior.
Sorry, I meant &&: `IsPythonObjectValid() && Base::IsValid()`. Basically, I'm trying to see if there's a reasonable way to reduce the number of classes floating around, and this `PresumptivelyValidFile` seems like it could be avoided...


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1251
+  PythonIOFile(const PythonFile &file, bool borrowed)
+      : OwnedPythonFile(file, borrowed){};
+
----------------
lawrence_danna wrote:
> labath wrote:
> > no semicolon
> huh?
There's no need to put a semicolon after the body of the constructor.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1278
+class BinaryPythonFile : public PythonIOFile {
+  friend class PythonFile;
+
----------------
lawrence_danna wrote:
> labath wrote:
> > What is this needed for? I don't see the PythonFile class doing anything funny (and it probably shouldn't).
> python API functions expect you to check for an exception with PyErr_Occured() and clear it before calling into them again.   If you don't things usually work out OK, except on a debug build in which an assert will trip.
Are you sure you're looking at the right place (the comments tend to move around as you reorganize code)? I was asking why is PythonFile a friend of the BinaryPythonFile class. That doesn't seem relevant to the PyErr_Occured checks...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68188