[Lldb-commits] [PATCH] Refactor Socket into a first class Host-object

[Zachary Turner]

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.


On Wed, Jul 30, 2014 at 6:16 PM, Greg Clayton <gclayton at apple.com> wrote:

> A few comments:
> - 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.
>
>  namespace lldb_private {
>
> +class Error;
> +class Socket;
>
>
> - Can we rename IoObject to IOObject? Or is there a better name?
> IOConnection? IOFile?
>
> - Add the "_sp" suffix to each of these and use IOObjectSP that you define
> in lldb-forward.h:
>
> +    std::shared_ptr<IoObject> m_read;
> +    std::shared_ptr<IoObject> m_write;
>
> - We have been using the "_ap" suffix for auto pointers, but we are slowly
> transitioning to using a "_up" for the unique pointers.
>
> +    std::unique_ptr<Socket> m_listening_socket; // For listen/accept
> connections, hold onto the
>
> 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:
>
> +    IoObject& GetReadObject() { return *m_read; }
> +    const IoObject& GetReadObject() const { return *m_read; }
>
>
> 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:
>
> +            if (read_object.GetFdType() == IoObject::eFDTypeSocket)
>              {
> -                char port_cstr[32];
> -                snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i",
> out_port);
> -                // Send the host and port down that debugserver and
> specify an option
> -                // so that it connects back to the port we are listening
> to in this process
> -                debugserver_args.AppendArgument("--reverse-connect");
> -                debugserver_args.AppendArgument(port_cstr);
> +                Socket& tcp_socket = static_cast<Socket&>(read_object);
> +                assert(tcp_socket.GetSocketProtocol() ==
> Socket::ProtocolTcp);
> +                out_port = tcp_socket.GetBoundPort(10);
>
> Other than that, lots of code has moved around into the host layer but it
> is mostly the same. So looks good.
>
> > On Jul 28, 2014, at 4:57 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > 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).
> >
> > 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.
> >
> > http://reviews.llvm.org/D4641
> >
> > Files:
> >  include/lldb/Core/ConnectionFileDescriptor.h
> >  include/lldb/Host/Editline.h
> >  include/lldb/Host/File.h
> >  include/lldb/Host/IoObject.h
> >  include/lldb/Host/Socket.h
> >  lldb.xcodeproj/project.pbxproj
> >  source/Core/ConnectionFileDescriptor.cpp
> >  source/Host/common/CMakeLists.txt
> >  source/Host/common/File.cpp
> >  source/Host/common/IoObject.cpp
> >  source/Host/common/Socket.cpp
> >  source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
> >  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
> >  tools/lldb-gdbserver/lldb-gdbserver.cpp
> >  tools/lldb-platform/lldb-platform.cpp
> > <D4641.11964.patch>_______________________________________________
> > lldb-commits mailing list
> > lldb-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140730/4b05e590/attachment.html>