teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
Could you move this function into the `Debugger` class and just make
`SBDebugger` forward to that function? We usually keep the `SB*` classes as
thin wrappers that only contain binding specific logic and have the actual
implementation in the sibling class without the `SB` prefix.
The only other thing that's missing is adding this new function to the
`./lldb/bindings/interface/SBDebugger.i` interface file (which SWIG is using to
generate Python bindings) and add a `/API/` test for it. It's enough to pipe
something like `version\nhelp\n` into the `self.dbg` instance and check the
output (and then try with a default-constructed SBStream too).
Beside that this looks fine. Thanks for fixing this, that is a really nasty
bug. I'll make a patch that adds a notice to all the existing `fd`/`FILE` based
SB APIs so that the next person that will be hit by this doesn't have to track
this down to this wonky behaviour.
================
Comment at: lldb/source/API/SBDebugger.cpp:364
+
+ ssize_t nrwr = write(fds[WRITE], stream.GetData(), stream.GetSize());
+ if (size_t(nrwr) != stream.GetSize()) {
----------------
I think this is unspecified behaviour for a default constructed `SBStream`, so
please make an early-exit at the top when `!stream.IsValid()`
================
Comment at: lldb/source/API/SBDebugger.cpp:394
+ return result;
+ }
+
----------------
no `{}` because it's just one line.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104413/new/
https://reviews.llvm.org/D104413
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
