[Lldb-commits] [PATCH] D68737: SBFile::GetFile: convert SBFile back into python native files.

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

lawrence_danna marked 3 inline comments as done.
lawrence_danna added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:781
+        with open(self.out_filename, 'r') as f:
+            # python2 returns None from write, python3 returns 7
+            lines = [x for x in f.read().strip().split() if x != "7"]
----------------
labath wrote:
> I find this comment confusing. Does that refer to the write call above? If so, I don't see how that is relevant here..
When we check the output on the next line we have to strip out the 7, which is the number of bytes written.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1576-1579
+  // Borrow the FILE*, the lldb_private::File still owns it
+  auto close = [](FILE *f) -> int { fflush(f); return 0; };
+  file_obj =
+      PyFile_FromFile(file.GetStream(), const_cast<char *>(""), cmode, close);
----------------
labath wrote:
> What would you say to passing `fflush` here directly? The signature is compatible with fclose, and fclose is documented to return (a superset of) the errors also reported by fflush. So, it seems like it might be useful to report flushing errors instead of swallowing them. Since accessing the FILE* after a fclose call (even if it fails) is UB, and the python code thinks it's calling fclose, it shouldn't mess up the FILE state in any way even if the flushing fails..
Oh yea good point


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68737