labath added inline comments.

Comment at: lldb/include/lldb/Host/File.h:56
   static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
+  static const char *GetStreamOpenModeFromOptions(uint32_t options);
lawrence_danna wrote:
> labath wrote:
> > change the argument type to OpenOptions. For some reason, lldb has 
> > historically used integer types for passing enumerations around, but we're 
> > now slowly changing that..
> Are you sure that's the right thing to do?   `eOpenOptionRead | 
> eOpenOptionWrite` has type `unsigned int`, not `OpenOptions`.
Ah, right, that was the reason... Regardless, I do thing that's right -- I'll 
continue the discussion on the other patch.

Comment at: lldb/include/lldb/Host/File.h:331-335
+  static char ID;
+  virtual bool isA(const void *classID) const { return classID == &ID; }
+  static bool classof(const File *file) { return file->isA(&ID); }
Please move this somewhere else -- somewhere near the top or bottom of the 
class maybe.. It does not make sense to have it in the middle of the open 
options stuff.

Comment at: 
-    @expectedFailure # FIXME implement SBFile::GetFile
     @skipIf(py_version=['<', (3,)])
lawrence_danna wrote:
> labath wrote:
> > So, if I understand correctly, these tests check that you can feed a file 
> > opened by python into lldb, and then pump it back out to python. Are there 
> > any tests which do the opposite (have lldb open a file, move it to python 
> > and then reinject it into lldb)?
> Yea, `foo is bar` in python is essentially a pointer comparison between the 
> `PyObject*` pointers, so this is testing that you get the same identical file 
> object back in the situations where you should.
> There's no test going the other way, because going the other way isn't 
> implemented.   I didn't write anything that could stash an arbitrary 
> `lldb_private::File` in a python object .   If you convert a non-python 
> `File`, you will get a new python file based on the descriptor, if it's 
> available, or the conversion will fail if one is not.   We do test that here, 
> on line 801, we're testing that a `NativeFile` is converted to a working 
> python file and the file descriptors match.
> We could, in a follow-on patch make the other direction work with identity 
> too, but right now I can't think of what it would be useful for.
Right, sorry, that came out a bit wrong. While I think it would be cool to have 
the other direction be an "identity" too, I don't think that is really 
necessary. Nevertheless, taking a File out of lldb and then back in will do 
_something_ right now, and that "something" could probably use a test. I 
suppose you could construct an SBFile from a path, convert it to a python file 
and back, and then ensure that reading/writing on those two SBFiles does 
something reasonable.

Comment at: lldb/scripts/interface/SBFile.i:81
+    %feature("docstring", "convert this SBFile into a python io.IOBase file 
+    FileSP GetFile();
lawrence_danna wrote:
> labath wrote:
> > I am somewhat surprised that there's no ownership handling in this patch 
> > (whereas ownership transfer was a fairly big chunk of the conversion in the 
> > other direction). Could you maybe explain what are the ownership 
> > expectations of the file returned through here (and what happens when we 
> > close it for instance)?
> Oh, yea good point.    `GetFile` returns a borrowed file, it shouldn't be 
> closed.   I'll add a comment explaining.
Cool, thanks. This description is very helpful.

Comment at: lldb/scripts/interface/SBFile.i:93
+    If there is no underlying python file to unwrap, GetFile will
+    use the file descirptor, if availble to create a new python
+    file object using `open(fd, mode=..., closefd=False)`

Comment at: 
+  auto *simple = llvm::dyn_cast<SimplePythonFile>(&file);
+  if (simple)
+    return Retain<PythonFile>(simple->GetPythonObject());
if (auto *simple = ...)

Comment at: 
+  PythonFile(File &file, const char *mode = nullptr) {
+    auto f = FromFile(file, mode);
+    if (f)
+      *this = std::move(f.get());
+    else {
+      Reset();
+      llvm::consumeError(f.takeError());
lawrence_danna wrote:
> labath wrote:
> > It looks like there's just one caller of this constructor (the "out" 
> > typemap for FILE*). Can we just inline this stuff there and delete this 
> > constructor?
> It's also called in some parts of ScriptInterpreterPython that are broken for 
> other reasons, and are getting fixed later in my patch queue.   I'll just put 
> a fixme here and delete it when all the other code that uses it also gets 
> deleted, ok?
Sounds good.

  rG LLVM Github Monorepo


lldb-commits mailing list

Reply via email to