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

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna marked 2 inline comments as done.
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:
> > > 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.
> Yeah, but that's where the second part of this idea comes in. The python IO files can re-override `IsValid()` so that it does *not* chain into the base class, and just calls `IsPythonObjectValid()` from `OwnedPythonFile`. That arrangement seems to make sense to me, though I am willing to be convinced otherwise.
oh, yea that works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68188