lawrence_danna 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; };
----------------
labath wrote:
> 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...
If i did it that way, with `&&` and no `PresumptivelyValidFile` then the python
IO files would chain into the base `File` class and say they're invalid.
That's how I coded it up at first, I didn't notice I needed
`PresumptivelyValidFile` until I saw the python IO files coming up as invalid.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1278
+class BinaryPythonFile : public PythonIOFile {
+ friend class PythonFile;
+
----------------
labath wrote:
> 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...
oh, yea it got moved around. fixed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68188/new/
https://reviews.llvm.org/D68188
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
