[Lldb-commits] [PATCH] Implements a HostThread class.

[Zachary Turner]

Latest version is up, should address all the issues pointed out in yoru
comments.  I didn't actually address the HostThread::GetName() issue in
this patch, because it turns out we only ever attempt to get the thread
name in one place, and it's of the current thread.  I still am fine
changing this, but the CL is already pretty huge and it didn't seem too
urgent for the purposes of this change.


On Fri, Sep 5, 2014 at 9:17 AM, <jingham at apple.com> wrote:

> Yes, the "alternate" method sounds fine to me as well.
>
> Jim
>
>
> > On Sep 4, 2014, at 6:35 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Come to think of it I actually like my "alternate" method more than the
> way I've done it, because it means I don't need to duplicate handles in the
> assignment / copy-constructor of HostThreadWindows, because the only thing
> that will get copied is the shared_ptr.
> >
> >
> > On Thu, Sep 4, 2014 at 6:18 PM, Zachary Turner <zturner at google.com>
> wrote:
> >
> >
> >
> > On Thu, Sep 4, 2014 at 6:06 PM, <jingham at apple.com> wrote:
> >
> > > On Sep 4, 2014, at 5:57 PM, Zachary Turner <zturner at google.com> wrote:
> > >
> > > Regarding point #1: I'm still not sold on this.  Exposing only the
> HostThreadBase really complicates some things.  Some of the issues escape
> my mind right now, but one in particular stands out.  There are a couple of
> places where threads are copied.  I don't remember the exact file off the
> top of my head, but something about making a copy of a thread in case the
> thread is shutting down and nulls itself out.  With a pointer or reference
> to a base class, this can't be done without a virtual Clone() method, which
> is really kind of gross.
> >
> > I would like to see what is hard.  You are using a generic factory to
> make the HostThreads, your ThreadRunner, and then you are calling generic
> methods on them.  Maybe I'm too stuck on this, but it seems like we should
> keep host specific stuff out of generic lldb functionality, and enforce
> that with the compiler, not with buildbots on the lacking host failing
> sometime later on...  That just seems ugly to me.
> >
> > It's not that it's hard, I guess it's just a difference of opinion on
> what's the most ugly.  Consider the following block of code which uses raw
> lldb:thread_t's.
> >
> > lldb::thread_t backup = m_thread;
> > ...
> > m_thread = backup;
> >
> > With my patch, that becomes the following:
> >
> > HostThread backup = m_thread;
> > ...
> > m_thread = backup;
> >
> > With the base class method, that becomes the following:
> >
> > HostThreadBaseSP backup = m_thread->Clone();
> > ...
> > m_thread = backup->Clone();
> >
> > These look similar, but behind the scenes it's ugly.  You now need a
> pure virtual Clone() method on HostThreadBase (trivial to implement of
> course, but it's just code pollution).  You need to store by pointer
> instead of by value, which means you have to worry about null-checks as
> well.
> >
> > There is the issue you mention which is that only a buildbot will tell
> you if you use a platform-specific method, but my argument is just that
> this is strictly better than before, because before *nobody* would tell you
> when you used a platform-specific method.  It would just be a no-op.
> >
> > That said, I have another idea in case you're still opposed to the way
> I've done it.  Basically, just have HostThread.  Nothing derives from it.
> Its only member is a shared_ptr<HostNativeThread>.  HostNativeThread does
> have subclasses just as the current HostThreadBase does.  The methods of
> HostThread just forward their calls to HostNativeThread, but also
> HostThread provides a method called GetNativeThread() which returns
> HostNativeThread automatically cast to the most-derived type.
> >
> > This way, in generic code, as long as you don't write
> thread.GetNativeThread(), you're guaranteed to only be using generic
> methods.
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140905/30d78dbd/attachment.html>