<div dir="ltr">I actually fixed a few instances of the casting in the most recent patch, thanks for catching a few stragglers. I do intend to fix those because I don't like it either.</div><div class="gmail_extra"><br>
<br><div class="gmail_quote">On Wed, Jul 30, 2014 at 6:16 PM, Greg Clayton <span dir="ltr"><<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
A few comments:<br>
- Any lldb_private forward declarations should be added to "include/lldb/lldb-forward.h" and then that file should be included instead of inlining any forward declarations manually. Also if you are going to use shared pointers to then there is a section in lldb-forward.h at the bottom where you should declare them like the others.<br>
<br>
namespace lldb_private {<br>
<br>
+class Error;<br>
+class Socket;<br>
<br>
<br>
- Can we rename IoObject to IOObject? Or is there a better name? IOConnection? IOFile?<br>
<br>
- Add the "_sp" suffix to each of these and use IOObjectSP that you define in lldb-forward.h:<br>
<br>
+ std::shared_ptr<IoObject> m_read;<br>
+ std::shared_ptr<IoObject> m_write;<br>
<br>
- We have been using the "_ap" suffix for auto pointers, but we are slowly transitioning to using a "_up" for the unique pointers.<br>
<br>
+ std::unique_ptr<Socket> m_listening_socket; // For listen/accept connections, hold onto the<br>
<br>
These make me nervous and should probably return a pointer or a shared pointer, not a reference unless they are private and are known to be used only when these are valid:<br>
<br>
+ IoObject& GetReadObject() { return *m_read; }<br>
+ const IoObject& GetReadObject() const { return *m_read; }<br>
<br>
<br>
This kind of code seems like we are trying to make a new class IoObject, but don't want to correctly abstract it down to a virtual class that can do everything that we need of it. Seems like we should add a GetBoundPort(...) to the IoObject class and have it return something invalid instead of casting and having to know what the IoObject is and cast it:<br>
<br>
+ if (read_object.GetFdType() == IoObject::eFDTypeSocket)<br>
{<br>
- char port_cstr[32];<br>
- snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i", out_port);<br>
- // Send the host and port down that debugserver and specify an option<br>
- // so that it connects back to the port we are listening to in this process<br>
- debugserver_args.AppendArgument("--reverse-connect");<br>
- debugserver_args.AppendArgument(port_cstr);<br>
+ Socket& tcp_socket = static_cast<Socket&>(read_object);<br>
+ assert(tcp_socket.GetSocketProtocol() == Socket::ProtocolTcp);<br>
+ out_port = tcp_socket.GetBoundPort(10);<br>
<br>
Other than that, lots of code has moved around into the host layer but it is mostly the same. So looks good.<br>
<div><div class="h5"><br>
> On Jul 28, 2014, at 4:57 PM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
><br>
> This fixes the crashing in lldb-gdbserver, and should include all of the necessary things to get it to compile on MacOSX as well. (I have a request for my own Mac workstation pending, until then I have to cross my fingers and hope I did it right).<br>
><br>
> lldb-gdbserver tests still fail, but now it's because of a pipe that's not getting any data. I suspect that all the remaining test failures now have the same underlying cause. Still investigating exactly what that might be, though.<br>
><br>
> <a href="http://reviews.llvm.org/D4641" target="_blank">http://reviews.llvm.org/D4641</a><br>
><br>
> Files:<br>
> include/lldb/Core/ConnectionFileDescriptor.h<br>
> include/lldb/Host/Editline.h<br>
> include/lldb/Host/File.h<br>
> include/lldb/Host/IoObject.h<br>
> include/lldb/Host/Socket.h<br>
> lldb.xcodeproj/project.pbxproj<br>
> source/Core/ConnectionFileDescriptor.cpp<br>
> source/Host/common/CMakeLists.txt<br>
> source/Host/common/File.cpp<br>
> source/Host/common/IoObject.cpp<br>
> source/Host/common/Socket.cpp<br>
> source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp<br>
> source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp<br>
> tools/lldb-gdbserver/lldb-gdbserver.cpp<br>
> tools/lldb-platform/lldb-platform.cpp<br>
</div></div>> <D4641.11964.patch>_______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
<br>
</blockquote></div><br></div>